[Qemu-devel] [PATCH v2 00/20] Improve bdrv_open error messages

[Qemu-devel] [PATCH v2 00/20] Improve bdrv_open error messages

[Paolo Bonzini]

Most of the block drivers are not using the Error** argument to
bdrv_open, and instead just printing errors to stderr.  This series
improves that, using Error** instead of abusing errno numbers too.

The only hurdle (caught by qemu-iotests, too) is VMDK, where we
currently parse a file first as image, and second as a descriptor.
This makes it hard to pick a good error message, because you do not
know which attempt gave the most reasonable error message.  For
this reason patches 15-17 modify this approach and push up the
detection of vmdk image file magic numbers.

Apart from this, and from a segfault fixed by patch 7, the series
consists of mostly trivial patches.

Paolo

Paolo Bonzini (20):
  nbd: produce a better error if neither host nor port is passed
  nbd: correctly propagate errors
  nbd: inline tcp_socket_incoming_spec into sole caller
  nbd: move socket wrappers to qemu-nbd
  iscsi: fix indentation
  iscsi: correctly propagate errors in iscsi_open
  gluster: default scheme to gluster:// and host to localhost.
  gluster: correctly propagate errors
  cow: correctly propagate errors
  curl: correctly propagate errors
  qcow: correctly propagate errors
  qed: correctly propagate errors
  vhdx: correctly propagate errors
  vvfat: correctly propagate errors
  vmdk: extract vmdk_read_desc
  vmdk: push vmdk_read_desc up to caller
  vmdk: do not try opening a file as both image and descriptor
  vmdk: correctly propagate errors
  block: do not abuse EMEDIUMTYPE
  vdi: say why an image is bad

 block/bochs.c              |   3 +-
 block/cow.c                |  15 ++---
 block/curl.c               |  13 ++---
 block/gluster.c            |  28 ++++-----
 block/iscsi.c              | 141 +++++++++++++++++++++++----------------------
 block/nbd.c                |  43 +++++++-------
 block/parallels.c          |   3 +-
 block/qcow.c               |  19 +++---
 block/qcow2.c              |   2 +-
 block/qed.c                |  19 +++---
 block/vdi.c                |  29 ++++++----
 block/vhdx.c               |  21 +++----
 block/vmdk.c               | 121 ++++++++++++++++++++++++--------------
 block/vpc.c                |   3 +-
 block/vvfat.c              |   9 +--
 include/block/nbd.h        |   6 --
 nbd.c                      |  66 ---------------------
 qemu-nbd.c                 |  52 +++++++++++++++++
 tests/qemu-iotests/059.out |   6 +-
 19 files changed, 302 insertions(+), 297 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 01/20] nbd: produce a better error if neither host nor port is passed

[Paolo Bonzini]

Before:
    $ qemu-io-old
    qemu-io-old> open -r -o file.driver=nbd
    qemu-io-old: can't open device (null): Could not open image: Invalid argument
    $ ./qemu-io-old
    qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
    path and host may not be used at the same time.
    qemu-io-old: can't open device (null): Could not open image: Invalid argument

After:
    $ ./qemu-io
    qemu-io> open -r -o file.driver=nbd
    one of path and host must be specified.
    qemu-io: can't open device (null): Could not open image: Invalid argument
    $ ./qemu-io
    qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
    path and host may not be used at the same time.
    qemu-io: can't open device (null): Could not open image: Invalid argument

Next patch will fix the error propagation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/nbd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 327e913..fd89083 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
 {
     Error *local_err = NULL;
 
-    if (qdict_haskey(options, "path")) {
-        if (qdict_haskey(options, "host")) {
+    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
+        if (qdict_haskey(options, "path")) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
                           "be used at the same time.");
-            return -EINVAL;
+        } else {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
+                          "must be specified.");
         }
-        s->client.is_unix = true;
-    } else if (qdict_haskey(options, "host")) {
-        s->client.is_unix = false;
-    } else {
         return -EINVAL;
     }
 
+    s->client.is_unix = qdict_haskey(options, "path");
     s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
                                       &error_abort);
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 02/20] nbd: correctly propagate errors

[Paolo Bonzini]

Before:
    $ ./qemu-io-old
    qemu-io-old> open -r -o file.driver=nbd
    one of path and host must be specified.
    qemu-io-old: can't open device (null): Could not open image: Invalid argument
    $ ./qemu-io-old
    qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
    path and host may not be used at the same time.
    qemu-io-old: can't open device (null): Could not open image: Invalid argument

After:
    $ ./qemu-io
    qemu-io> open -r -o file.driver=nbd
    qemu-io: can't open device (null): one of path and host must be specified.
    $ ./qemu-io
    qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
    qemu-io: can't open device (null): path and host may not be used at the same time.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/nbd.c         | 34 ++++++++++++++++------------------
 include/block/nbd.h |  1 -
 nbd.c               | 12 ------------
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index fd89083..69f336b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -188,19 +188,18 @@ out:
     g_free(file);
 }
 
-static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
+static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
+                       Error **errp)
 {
     Error *local_err = NULL;
 
     if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
         if (qdict_haskey(options, "path")) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
-                          "be used at the same time.");
+            error_setg(errp, "path and host may not be used at the same time.");
         } else {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
-                          "must be specified.");
+            error_setg(errp, "one of path and host must be specified.");
         }
-        return -EINVAL;
+        return;
     }
 
     s->client.is_unix = qdict_haskey(options, "path");
@@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
 
     qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -EINVAL;
+        error_propagate(errp, local_err);
+        return;
     }
 
     if (!qemu_opt_get(s->socket_opts, "port")) {
@@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
     if (*export) {
         qdict_del(options, "export");
     }
-
-    return 0;
 }
 
-static int nbd_establish_connection(BlockDriverState *bs)
+static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
 {
     BDRVNBDState *s = bs->opaque;
     int sock;
 
     if (s->client.is_unix) {
-        sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
+        sock = unix_connect_opts(s->socket_opts, errp, NULL, NULL);
     } else {
-        sock = tcp_socket_outgoing_opts(s->socket_opts);
+        sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
         if (sock >= 0) {
             socket_set_nodelay(sock);
         }
@@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVNBDState *s = bs->opaque;
     char *export = NULL;
     int result, sock;
+    Error *local_err = NULL;
 
     /* Pop the config into our state object. Exit if invalid. */
-    result = nbd_config(s, options, &export);
-    if (result != 0) {
-        return result;
+    nbd_config(s, options, &export, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return -EINVAL;
     }
 
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sock = nbd_establish_connection(bs);
+    sock = nbd_establish_connection(bs, errp);
     if (sock < 0) {
         return sock;
     }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index c90f5e4..e10ab82 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -64,7 +64,6 @@ enum {
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
 int tcp_socket_incoming(const char *address, uint16_t port);
 int tcp_socket_incoming_spec(const char *address_and_port);
-int tcp_socket_outgoing_opts(QemuOpts *opts);
 int unix_socket_outgoing(const char *path);
 int unix_socket_incoming(const char *path);
 
diff --git a/nbd.c b/nbd.c
index 030f56b..17ca95b 100644
--- a/nbd.c
+++ b/nbd.c
@@ -199,18 +199,6 @@ static void combine_addr(char *buf, size_t len, const char* address,
     }
 }
 
-int tcp_socket_outgoing_opts(QemuOpts *opts)
-{
-    Error *local_err = NULL;
-    int fd = inet_connect_opts(opts, &local_err, NULL, NULL);
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-
-    return fd;
-}
-
 int tcp_socket_incoming(const char *address, uint16_t port)
 {
     char address_and_port[128];
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 03/20] nbd: inline tcp_socket_incoming_spec into sole caller

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/nbd.h | 1 -
 nbd.c               | 8 ++------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index e10ab82..1b39c06 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -63,7 +63,6 @@ enum {
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
 int tcp_socket_incoming(const char *address, uint16_t port);
-int tcp_socket_incoming_spec(const char *address_and_port);
 int unix_socket_outgoing(const char *path);
 int unix_socket_incoming(const char *path);
 
diff --git a/nbd.c b/nbd.c
index 17ca95b..2fc1f1f 100644
--- a/nbd.c
+++ b/nbd.c
@@ -202,13 +202,9 @@ static void combine_addr(char *buf, size_t len, const char* address,
 int tcp_socket_incoming(const char *address, uint16_t port)
 {
     char address_and_port[128];
-    combine_addr(address_and_port, 128, address, port);
-    return tcp_socket_incoming_spec(address_and_port);
-}
-
-int tcp_socket_incoming_spec(const char *address_and_port)
-{
     Error *local_err = NULL;
+
+    combine_addr(address_and_port, 128, address, port);
     int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
 
     if (local_err != NULL) {
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 04/20] nbd: move socket wrappers to qemu-nbd

[Paolo Bonzini]

qemu-nbd is one of the few valid users of qerror_report_err.  Move
the error-reporting socket wrappers there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/nbd.h |  4 ----
 nbd.c               | 50 --------------------------------------------------
 qemu-nbd.c          | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 54 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1b39c06..79502a0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -62,10 +62,6 @@ enum {
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int tcp_socket_incoming(const char *address, uint16_t port);
-int unix_socket_outgoing(const char *path);
-int unix_socket_incoming(const char *path);
-
 int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
                           off_t *size, size_t *blocksize);
 int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize);
diff --git a/nbd.c b/nbd.c
index 2fc1f1f..e5084b6 100644
--- a/nbd.c
+++ b/nbd.c
@@ -188,56 +188,6 @@ static ssize_t write_sync(int fd, void *buffer, size_t size)
     return ret;
 }
 
-static void combine_addr(char *buf, size_t len, const char* address,
-                         uint16_t port)
-{
-    /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
-    if (strstr(address, ":")) {
-        snprintf(buf, len, "[%s]:%u", address, port);
-    } else {
-        snprintf(buf, len, "%s:%u", address, port);
-    }
-}
-
-int tcp_socket_incoming(const char *address, uint16_t port)
-{
-    char address_and_port[128];
-    Error *local_err = NULL;
-
-    combine_addr(address_and_port, 128, address, port);
-    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
-
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    return fd;
-}
-
-int unix_socket_incoming(const char *path)
-{
-    Error *local_err = NULL;
-    int fd = unix_listen(path, NULL, 0, &local_err);
-
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    return fd;
-}
-
-int unix_socket_outgoing(const char *path)
-{
-    Error *local_err = NULL;
-    int fd = unix_connect(path, &local_err);
-
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    return fd;
-}
-
 /* Basic flow for negotiation
 
    Server         Client
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 136e8c9..8138435 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -20,6 +20,8 @@
 #include "block/block.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
+#include "qemu/sockets.h"
+#include "qemu/error-report.h"
 #include "block/snapshot.h"
 
 #include <stdarg.h>
@@ -201,6 +203,56 @@ static void termsig_handler(int signum)
     qemu_notify_event();
 }
 
+static void combine_addr(char *buf, size_t len, const char* address,
+                         uint16_t port)
+{
+    /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
+    if (strstr(address, ":")) {
+        snprintf(buf, len, "[%s]:%u", address, port);
+    } else {
+        snprintf(buf, len, "%s:%u", address, port);
+    }
+}
+
+static int tcp_socket_incoming(const char *address, uint16_t port)
+{
+    char address_and_port[128];
+    Error *local_err = NULL;
+
+    combine_addr(address_and_port, 128, address, port);
+    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
+}
+
+static int unix_socket_incoming(const char *path)
+{
+    Error *local_err = NULL;
+    int fd = unix_listen(path, NULL, 0, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
+}
+
+static int unix_socket_outgoing(const char *path)
+{
+    Error *local_err = NULL;
+    int fd = unix_connect(path, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
+}
+
 static void *show_parts(void *arg)
 {
     char *device = arg;
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 05/20] iscsi: fix indentation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6f4af72..e654a57 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1065,35 +1065,36 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
-                                          int lun, int evpd, int pc) {
-        int full_size;
-        struct scsi_task *task = NULL;
-        task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
+                                          int evpd, int pc)
+{
+    int full_size;
+    struct scsi_task *task = NULL;
+    task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+    if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+        goto fail;
+    }
+    full_size = scsi_datain_getfullsize(task);
+    if (full_size > task->datain.size) {
+        scsi_free_scsi_task(task);
+
+        /* we need more data for the full list */
+        task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
         if (task == NULL || task->status != SCSI_STATUS_GOOD) {
             goto fail;
         }
-        full_size = scsi_datain_getfullsize(task);
-        if (full_size > task->datain.size) {
-            scsi_free_scsi_task(task);
-
-            /* we need more data for the full list */
-            task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
-            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-                goto fail;
-            }
-        }
+    }
 
-        return task;
+    return task;
 
 fail:
-        error_report("iSCSI: Inquiry command failed : %s",
-                     iscsi_get_error(iscsi));
-        if (task) {
-            scsi_free_scsi_task(task);
-            return NULL;
-        }
+    error_report("iSCSI: Inquiry command failed : %s",
+                 iscsi_get_error(iscsi));
+    if (task) {
+        scsi_free_scsi_task(task);
         return NULL;
+    }
+    return NULL;
 }
 
 /*
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 06/20] iscsi: correctly propagate errors in iscsi_open

[Paolo Bonzini]

Before:
    $ ./qemu-io-old
    qemu-io-old> open -r -o file.driver=iscsi,file.filename=foo
    Failed to parse URL : foo
    qemu-io-old: can't open device (null): Could not open 'foo': Invalid argument

After:
    $ ./qemu-io
    qemu-io> open -r -o file.driver=iscsi,file.filename=foo
    qemu-io: can't open device (null): Failed to parse URL : foo

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 102 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index e654a57..2cf1983 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -856,7 +856,8 @@ retry:
 
 #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
 
-static int parse_chap(struct iscsi_context *iscsi, const char *target)
+static void parse_chap(struct iscsi_context *iscsi, const char *target,
+                       Error **errp)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -865,37 +866,35 @@ static int parse_chap(struct iscsi_context *iscsi, const char *target)
 
     list = qemu_find_opts("iscsi");
     if (!list) {
-        return 0;
+        return;
     }
 
     opts = qemu_opts_find(list, target);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
-            return 0;
+            return;
         }
     }
 
     user = qemu_opt_get(opts, "user");
     if (!user) {
-        return 0;
+        return;
     }
 
     password = qemu_opt_get(opts, "password");
     if (!password) {
-        error_report("CHAP username specified but no password was given");
-        return -1;
+        error_setg(errp, "CHAP username specified but no password was given");
+        return;
     }
 
     if (iscsi_set_initiator_username_pwd(iscsi, user, password)) {
-        error_report("Failed to set initiator username and password");
-        return -1;
+        error_setg(errp, "Failed to set initiator username and password");
     }
-
-    return 0;
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
+static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+                                Error **errp)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -928,7 +927,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
     } else if (!strcmp(digest, "NONE-CRC32C")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
     } else {
-        error_report("Invalid header-digest setting : %s", digest);
+        error_setg(errp, "Invalid header-digest setting : %s", digest);
     }
 }
 
@@ -986,12 +985,11 @@ static void iscsi_nop_timed_event(void *opaque)
 }
 #endif
 
-static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
+static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
 {
     struct scsi_task *task = NULL;
     struct scsi_readcapacity10 *rc10 = NULL;
     struct scsi_readcapacity16 *rc16 = NULL;
-    int ret = 0;
     int retries = ISCSI_CMD_RETRIES; 
 
     do {
@@ -1006,8 +1004,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             if (task != NULL && task->status == SCSI_STATUS_GOOD) {
                 rc16 = scsi_datain_unmarshall(task);
                 if (rc16 == NULL) {
-                    error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
-                    ret = -EINVAL;
+                    error_setg(errp, "iSCSI: Failed to unmarshall readcapacity16 data.");
                 } else {
                     iscsilun->block_size = rc16->block_length;
                     iscsilun->num_blocks = rc16->returned_lba + 1;
@@ -1021,8 +1018,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             if (task != NULL && task->status == SCSI_STATUS_GOOD) {
                 rc10 = scsi_datain_unmarshall(task);
                 if (rc10 == NULL) {
-                    error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
-                    ret = -EINVAL;
+                    error_setg(errp, "iSCSI: Failed to unmarshall readcapacity10 data.");
                 } else {
                     iscsilun->block_size = rc10->block_size;
                     if (rc10->lba == 0) {
@@ -1035,20 +1031,18 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             }
             break;
         default:
-            return 0;
+            return;
         }
     } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
              && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
              && retries-- > 0);
 
     if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-        error_report("iSCSI: failed to send readcapacity10 command.");
-        ret = -EINVAL;
+        error_setg(errp, "iSCSI: failed to send readcapacity10 command.");
     }
     if (task) {
         scsi_free_scsi_task(task);
     }
-    return ret;
 }
 
 /* TODO Convert to fine grained options */
@@ -1066,7 +1060,7 @@ static QemuOptsList runtime_opts = {
 };
 
 static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
-                                          int evpd, int pc)
+                                          int evpd, int pc, Error **errp)
 {
     int full_size;
     struct scsi_task *task = NULL;
@@ -1088,8 +1082,8 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
     return task;
 
 fail:
-    error_report("iSCSI: Inquiry command failed : %s",
-                 iscsi_get_error(iscsi));
+    error_setg(errp, "iSCSI: Inquiry command failed : %s",
+               iscsi_get_error(iscsi));
     if (task) {
         scsi_free_scsi_task(task);
         return NULL;
@@ -1116,27 +1110,25 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
     if ((BDRV_SECTOR_SIZE % 512) != 0) {
-        error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
-                     "BDRV_SECTOR_SIZE(%lld) is not a multiple "
-                     "of 512", BDRV_SECTOR_SIZE);
+        error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
+                   "BDRV_SECTOR_SIZE(%lld) is not a multiple "
+                   "of 512", BDRV_SECTOR_SIZE);
         return -EINVAL;
     }
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     filename = qemu_opt_get(opts, "filename");
 
-
     iscsi_url = iscsi_parse_full_url(iscsi, filename);
     if (iscsi_url == NULL) {
-        error_report("Failed to parse URL : %s", filename);
+        error_setg(errp, "Failed to parse URL : %s", filename);
         ret = -EINVAL;
         goto out;
     }
@@ -1147,13 +1139,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
-        error_report("iSCSI: Failed to create iSCSI context.");
+        error_setg(errp, "iSCSI: Failed to create iSCSI context.");
         ret = -ENOMEM;
         goto out;
     }
 
     if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
-        error_report("iSCSI: Failed to set target name.");
+        error_setg(errp, "iSCSI: Failed to set target name.");
         ret = -EINVAL;
         goto out;
     }
@@ -1162,21 +1154,22 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user,
                                               iscsi_url->passwd);
         if (ret != 0) {
-            error_report("Failed to set initiator username and password");
+            error_setg(errp, "Failed to set initiator username and password");
             ret = -EINVAL;
             goto out;
         }
     }
 
     /* check if we got CHAP username/password via the options */
-    if (parse_chap(iscsi, iscsi_url->target) != 0) {
-        error_report("iSCSI: Failed to set CHAP user/password");
+    parse_chap(iscsi, iscsi_url->target, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
-        error_report("iSCSI: Failed to set session type to normal.");
+        error_setg(errp, "iSCSI: Failed to set session type to normal.");
         ret = -EINVAL;
         goto out;
     }
@@ -1184,10 +1177,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
 
     /* check if we got HEADER_DIGEST via the options */
-    parse_header_digest(iscsi, iscsi_url->target);
+    parse_header_digest(iscsi, iscsi_url->target, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto out;
+    }
 
     if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
-        error_report("iSCSI: Failed to connect to LUN : %s",
+        error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
             iscsi_get_error(iscsi));
         ret = -EINVAL;
         goto out;
@@ -1199,14 +1197,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 0, 0, 36);
 
     if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-        error_report("iSCSI: failed to send inquiry command.");
+        error_setg(errp, "iSCSI: failed to send inquiry command.");
         ret = -EINVAL;
         goto out;
     }
 
     inq = scsi_datain_unmarshall(task);
     if (inq == NULL) {
-        error_report("iSCSI: Failed to unmarshall inquiry data.");
+        error_setg(errp, "iSCSI: Failed to unmarshall inquiry data.");
         ret = -EINVAL;
         goto out;
     }
@@ -1214,7 +1212,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsilun->type = inq->periperal_device_type;
     iscsilun->has_write_same = true;
 
-    if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
+    iscsi_readcapacity_sync(iscsilun, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
         goto out;
     }
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
@@ -1232,14 +1232,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     if (iscsilun->lbpme) {
         struct scsi_inquiry_logical_block_provisioning *inq_lbp;
         task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
-                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
+                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
+                                errp);
         if (task == NULL) {
             ret = -EINVAL;
             goto out;
         }
         inq_lbp = scsi_datain_unmarshall(task);
         if (inq_lbp == NULL) {
-            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
             ret = -EINVAL;
             goto out;
         }
@@ -1252,14 +1253,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
         struct scsi_inquiry_block_limits *inq_bl;
         task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
-                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS, errp);
         if (task == NULL) {
             ret = -EINVAL;
             goto out;
         }
         inq_bl = scsi_datain_unmarshall(task);
         if (inq_bl == NULL) {
-            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
             ret = -EINVAL;
             goto out;
         }
@@ -1349,14 +1350,15 @@ static int iscsi_reopen_prepare(BDRVReopenState *state,
 static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 {
     IscsiLun *iscsilun = bs->opaque;
-    int ret = 0;
+    Error *local_err = NULL;
 
     if (iscsilun->type != TYPE_DISK) {
         return -ENOTSUP;
     }
 
-    if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
-        return ret;
+    iscsi_readcapacity_sync(iscsilun, &local_err);
+    if (local_err != NULL) {
+        return -EIO;
     }
 
     if (offset > iscsi_getlength(bs)) {
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 07/20] gluster: default scheme to gluster:// and host to localhost.

[Paolo Bonzini]

Currently, "gluster:///volname/img" and (using file. options)
"file.driver=gluster,file.filename=foo" will segfault.  Also,
"//host/volname/img" will be rejected, but it is a valid URL
that should be accepted just fine with "file.driver=gluster".
Accept all of these, by inferring missing transport and host
as TCP and localhost respectively.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/gluster.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index a009b15..f9dd37f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -127,7 +127,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
     }
 
     /* transport */
-    if (!strcmp(uri->scheme, "gluster")) {
+    if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
         gconf->transport = g_strdup("tcp");
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
         gconf->transport = g_strdup("tcp");
@@ -163,7 +163,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
         }
         gconf->server = g_strdup(qp->p[0].value);
     } else {
-        gconf->server = g_strdup(uri->server);
+        gconf->server = g_strdup(uri->server ? uri->server : "localhost");
         gconf->port = uri->port;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 08/20] gluster: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/gluster.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index f9dd37f..bc9c59f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -175,7 +175,8 @@ out:
     return ret;
 }
 
-static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
+                                      Error **errp)
 {
     struct glfs *glfs = NULL;
     int ret;
@@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
 
     ret = qemu_gluster_parseuri(gconf, filename);
     if (ret < 0) {
-        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
-            "volname/image[?socket=...]");
+        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
+                   "volname/image[?socket=...]");
         errno = -ret;
         goto out;
     }
@@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
 
     ret = glfs_init(glfs);
     if (ret) {
-        error_report("Gluster connection failed for server=%s port=%d "
-             "volume=%s image=%s transport=%s", gconf->server, gconf->port,
-             gconf->volname, gconf->image, gconf->transport);
+        error_setg_errno(errp, errno,
+                         "Gluster connection failed for server=%s port=%d "
+                         "volume=%s image=%s transport=%s", gconf->server,
+                         gconf->port, gconf->volname, gconf->image,
+                         gconf->transport);
         goto out;
     }
     return glfs;
@@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     filename = qemu_opt_get(opts, "filename");
 
-    s->glfs = qemu_gluster_init(gconf, filename);
+    s->glfs = qemu_gluster_init(gconf, filename, errp);
     if (!s->glfs) {
         ret = -errno;
         goto out;
@@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
     int64_t total_size = 0;
     GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
 
-    glfs = qemu_gluster_init(gconf, filename);
+    glfs = qemu_gluster_init(gconf, filename, errp);
     if (!glfs) {
-        ret = -errno;
+        ret = -EINVAL;
         goto out;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..43a2150 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
         char version[64];
         snprintf(version, sizeof(version),
                "COW version %d", cow_header.version);
-        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
             bs->device_name, "cow", version);
         ret = -ENOTSUP;
         goto fail;
@@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
     struct stat st;
     int64_t image_sectors = 0;
     const char *image_filename = NULL;
-    Error *local_err = NULL;
     int ret;
     BlockDriverState *cow_bs;
 
@@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options, &local_err);
+    ret = bdrv_create_file(filename, options, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
-                         &local_err);
+    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 10/20] curl: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/curl.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index a807584..353bab1 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -456,30 +456,27 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     static int inited = 0;
 
     if (flags & BDRV_O_RDWR) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "curl block device does not support writes");
+        error_setg(errp, "curl block device does not support writes");
         return -EROFS;
     }
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         goto out_noclean;
     }
 
     s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
     if ((s->readahead_size & 0x1ff) != 0) {
-        fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
-                s->readahead_size);
+        error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512",
+                   s->readahead_size);
         goto out_noclean;
     }
 
     file = qemu_opt_get(opts, "url");
     if (file == NULL) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires "
-                      "an 'url' option");
+        error_setg(errp, "curl block driver requires an 'url' option");
         goto out_noclean;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 11/20] qcow: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..23bc691 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -119,17 +119,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     if (header.version != QCOW_VERSION) {
         char version[64];
         snprintf(version, sizeof(version), "QCOW version %d", header.version);
-        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-            bs->device_name, "qcow", version);
+        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+                  bs->device_name, "qcow", version);
         ret = -ENOTSUP;
         goto fail;
     }
 
     if (header.size <= 1 || header.cluster_bits < 9) {
+        error_setg(errp, "invalid value in qcow header");
         ret = -EINVAL;
         goto fail;
     }
     if (header.crypt_method > QCOW_CRYPT_AES) {
+        error_setg(errp, "invalid encryption method in qcow header");
         ret = -EINVAL;
         goto fail;
     }
@@ -668,7 +670,6 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
     int64_t total_size = 0;
     const char *backing_file = NULL;
     int flags = 0;
-    Error *local_err = NULL;
     int ret;
     BlockDriverState *qcow_bs;
 
@@ -684,18 +685,13 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options, &local_err);
+    ret = bdrv_create_file(filename, options, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
-                         &local_err);
+    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 12/20] qed: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qed.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 694e6e2..59bdd58 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -398,7 +398,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
         char buf[64];
         snprintf(buf, sizeof(buf), "%" PRIx64,
             s->header.features & ~QED_FEATURE_MASK);
-        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
             bs->device_name, "QED", buf);
         return -ENOTSUP;
     }
@@ -545,7 +545,8 @@ static void bdrv_qed_close(BlockDriverState *bs)
 
 static int qed_create(const char *filename, uint32_t cluster_size,
                       uint64_t image_size, uint32_t table_size,
-                      const char *backing_file, const char *backing_fmt)
+                      const char *backing_file, const char *backing_fmt,
+                      Error **errp)
 {
     QEDHeader header = {
         .magic = QED_MAGIC,
@@ -560,22 +561,17 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     QEDHeader le_header;
     uint8_t *l1_table = NULL;
     size_t l1_size = header.cluster_size * header.table_size;
-    Error *local_err = NULL;
     int ret = 0;
     BlockDriverState *bs = NULL;
 
-    ret = bdrv_create_file(filename, NULL, &local_err);
+    ret = bdrv_create_file(filename, NULL, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
     ret = bdrv_file_open(&bs, filename, NULL, NULL,
-                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
+                         BDRV_O_RDWR | BDRV_O_CACHE_WB, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return ret;
     }
 
@@ -665,7 +661,7 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options,
     }
 
     return qed_create(filename, cluster_size, image_size, table_size,
-                      backing_file, backing_fmt);
+                      backing_file, backing_fmt, errp);
 }
 
 typedef struct {
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 13/20] vhdx: correctly propagate errors

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vhdx.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 9ee0a61..de1a80a 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -402,9 +402,10 @@ int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s,
 }
 
 /* opens the specified header block from the VHDX file header section */
-static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
+static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
+                              Error **errp)
 {
-    int ret = 0;
+    int ret;
     VHDXHeader *header1;
     VHDXHeader *header2;
     bool h1_valid = false;
@@ -462,7 +463,6 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
     } else if (!h1_valid && h2_valid) {
         s->curr_header = 1;
     } else if (!h1_valid && !h2_valid) {
-        ret = -EINVAL;
         goto fail;
     } else {
         /* If both headers are valid, then we choose the active one by the
@@ -473,27 +473,22 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
         } else if (h2_seq > h1_seq) {
             s->curr_header = 1;
         } else {
-            ret = -EINVAL;
             goto fail;
         }
     }
 
     vhdx_region_register(s, s->headers[s->curr_header]->log_offset,
                             s->headers[s->curr_header]->log_length);
-
-    ret = 0;
-
     goto exit;
 
 fail:
-    qerror_report(ERROR_CLASS_GENERIC_ERROR, "No valid VHDX header found");
+    error_setg_errno(errp, -ret, "No valid VHDX header found");
     qemu_vfree(header1);
     qemu_vfree(header2);
     s->headers[0] = NULL;
     s->headers[1] = NULL;
 exit:
     qemu_vfree(buffer);
-    return ret;
 }
 
 
@@ -878,7 +873,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     int ret = 0;
     uint32_t i;
     uint64_t signature;
-
+    Error *local_err = NULL;
 
     s->bat = NULL;
     s->first_visible_write = true;
@@ -901,8 +896,10 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
      * header update */
     vhdx_guid_generate(&s->session_guid);
 
-    ret = vhdx_parse_header(bs, s);
-    if (ret < 0) {
+    vhdx_parse_header(bs, s, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
         goto fail;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 14/20] vvfat: correctly propagate errors

[Paolo Bonzini]

Before:
    $ ./qemu-io-old
    qemu-io-old> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
    Valid FAT types are only 12, 16 and 32
    qemu-io-old: can't open device (null): Could not open image: Invalid argument

After:
    $ ./qemu-io
    qemu-io> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu
    qemu-io: can't open device (null): Valid FAT types are only 12, 16 and 32

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 664941c..7c3521a 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1086,16 +1086,14 @@ DLOG(if (stderr == NULL) {
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto fail;
     }
 
     dirname = qemu_opt_get(opts, "dir");
     if (!dirname) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "vvfat block driver requires "
-                      "a 'dir' option");
+        error_setg(errp, "vvfat block driver requires a 'dir' option");
         ret = -EINVAL;
         goto fail;
     }
@@ -1135,8 +1133,7 @@ DLOG(if (stderr == NULL) {
     case 12:
         break;
     default:
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Valid FAT types are only "
-                      "12, 16 and 32");
+        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
         ret = -EINVAL;
         goto fail;
     }
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 15/20] vmdk: extract vmdk_read_desc

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 99ca60f..58f4c34 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -529,6 +529,32 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
                                uint64_t desc_offset, Error **errp);
 
+static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
+                            Error **errp)
+{
+    int64_t size;
+    char *buf;
+    int ret;
+
+    size = bdrv_getlength(file);
+    if (size < 0) {
+        error_setg_errno(errp, -size, "Could not access file");
+        return NULL;
+    }
+
+    size = MIN(size, 1 << 20);  /* avoid unbounded allocation */
+    buf = g_malloc0(size + 1);
+
+    ret = bdrv_pread(file, desc_offset, buf, size);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read from file");
+        g_free(buf);
+        return NULL;
+    }
+
+    return buf;
+}
+
 static int vmdk_open_vmdk4(BlockDriverState *bs,
                            BlockDriverState *file,
                            int flags, Error **errp)
@@ -822,23 +847,16 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
                                uint64_t desc_offset, Error **errp)
 {
     int ret;
-    char *buf = NULL;
+    char *buf;
     char ct[128];
     BDRVVmdkState *s = bs->opaque;
-    int64_t size;
 
-    size = bdrv_getlength(bs->file);
-    if (size < 0) {
+    buf = vmdk_read_desc(bs->file, desc_offset, errp);
+    if (!buf) {
         return -EINVAL;
-    }
-
-    size = MIN(size, 1 << 20);  /* avoid unbounded allocation */
-    buf = g_malloc0(size + 1);
-
-    ret = bdrv_pread(bs->file, desc_offset, buf, size);
-    if (ret < 0) {
         goto exit;
     }
+
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
         ret = -EMEDIUMTYPE;
         goto exit;
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 16/20] vmdk: push vmdk_read_desc up to caller

[Paolo Bonzini]

Currently, we just try reading a VMDK file as both image and descriptor.
This makes it hard to choose which of the two attempts gave the best error.
We'll decide in advance if the file looks like an image or a descriptor,
and this patch is the first step to that end.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c | 54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 58f4c34..74a0bac 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -526,8 +526,8 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
     return ret;
 }
 
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
-                               uint64_t desc_offset, Error **errp);
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
+                               Error **errp);
 
 static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
                             Error **errp)
@@ -575,7 +575,13 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (header.capacity == 0) {
         uint64_t desc_offset = le64_to_cpu(header.desc_offset);
         if (desc_offset) {
-            return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp);
+            char *buf = vmdk_read_desc(file, desc_offset << 9, errp);
+            if (!buf) {
+                return -EINVAL;
+            }
+            ret = vmdk_open_desc_file(bs, flags, buf, errp);
+            g_free(buf);
+            return ret;
         }
     }
 
@@ -726,16 +732,12 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
 
 /* Open an extent file and append to bs array */
 static int vmdk_open_sparse(BlockDriverState *bs,
-                            BlockDriverState *file,
-                            int flags, Error **errp)
+                            BlockDriverState *file, int flags,
+                            char *buf, Error **errp)
 {
     uint32_t magic;
 
-    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
-        return -EIO;
-    }
-
-    magic = be32_to_cpu(magic);
+    magic = ldl_be_p(buf);
     switch (magic) {
         case VMDK3_MAGIC:
             return vmdk_open_vmfs_sparse(bs, file, flags, errp);
@@ -819,8 +821,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
             /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse file*/
-            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp);
+            char *buf = vmdk_read_desc(extent_file, 0, errp);
+            if (!buf) {
+                ret = -EINVAL;
+            } else {
+                ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, buf, errp);
+            }
             if (ret) {
+                g_free(buf);
                 bdrv_unref(extent_file);
                 return ret;
             }
@@ -843,20 +850,13 @@ next_line:
     return 0;
 }
 
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
-                               uint64_t desc_offset, Error **errp)
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
+                               Error **errp)
 {
     int ret;
-    char *buf;
     char ct[128];
     BDRVVmdkState *s = bs->opaque;
 
-    buf = vmdk_read_desc(bs->file, desc_offset, errp);
-    if (!buf) {
-        return -EINVAL;
-        goto exit;
-    }
-
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
         ret = -EMEDIUMTYPE;
         goto exit;
@@ -874,20 +874,25 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
     s->desc_offset = 0;
     ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
 exit:
-    g_free(buf);
     return ret;
 }
 
 static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
+    char *buf = NULL;
     int ret;
     BDRVVmdkState *s = bs->opaque;
 
-    if (vmdk_open_sparse(bs, bs->file, flags, errp) == 0) {
+    buf = vmdk_read_desc(bs->file, 0, errp);
+    if (!buf) {
+        return -EINVAL;
+    }
+
+    if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) {
         s->desc_offset = 0x200;
     } else {
-        ret = vmdk_open_desc_file(bs, flags, 0, errp);
+        ret = vmdk_open_desc_file(bs, flags, buf, errp);
         if (ret) {
             goto fail;
         }
@@ -906,10 +911,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
               "vmdk", bs->device_name, "live migration");
     migrate_add_blocker(s->migration_blocker);
-
+    g_free(buf);
     return 0;
 
 fail:
+    g_free(buf);
     g_free(s->create_type);
     s->create_type = NULL;
     vmdk_free_extents(bs);
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 17/20] vmdk: do not try opening a file as both image and descriptor

[Paolo Bonzini]

This prepares for propagating errors from vmdk_open_sparse and
vmdk_open_desc_file up to the caller of vmdk_open.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c               | 22 +++++++++++++++-------
 tests/qemu-iotests/059.out |  4 ++--
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 74a0bac..750e632 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -883,20 +883,28 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
     char *buf = NULL;
     int ret;
     BDRVVmdkState *s = bs->opaque;
+    uint32_t magic;
 
     buf = vmdk_read_desc(bs->file, 0, errp);
     if (!buf) {
         return -EINVAL;
     }
 
-    if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) {
-        s->desc_offset = 0x200;
-    } else {
-        ret = vmdk_open_desc_file(bs, flags, buf, errp);
-        if (ret) {
-            goto fail;
-        }
+    magic = ldl_be_p(buf);
+    switch (magic) {
+        case VMDK3_MAGIC:
+        case VMDK4_MAGIC:
+            ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp);
+            s->desc_offset = 0x200;
+            break;
+        default:
+            ret = vmdk_open_desc_file(bs, flags, buf, errp);
+            break;
     }
+    if (ret) {
+        goto fail;
+    }
+
     /* try to open parent images, if exist */
     ret = vmdk_parent_open(bs);
     if (ret) {
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4ffeb54..4600670 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -8,7 +8,7 @@ no file open, try 'help open'
 === Testing too big L2 table size ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 L2 table size too big
-qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Wrong medium type
+qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
 no file open, try 'help open'
 
 === Testing too big L1 table size ===
@@ -2046,7 +2046,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
 === Testing truncated sparse ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
 qemu-img: File truncated, expecting at least 13172736 bytes
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Wrong medium type
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
 
 === Testing version 3 ===
 image: TEST_DIR/iotest-version3.IMGFMT
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 18/20] vmdk: correctly propagate errors

[Paolo Bonzini]

Now that we can return the "right" errors, use the Error** parameter
to pass them back instead of just printing them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c               | 11 ++++++-----
 tests/qemu-iotests/059.out |  6 ++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 750e632..f148164 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -571,6 +571,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
                          file->filename);
+        return -EINVAL;
     }
     if (header.capacity == 0) {
         uint64_t desc_offset = le64_to_cpu(header.desc_offset);
@@ -640,8 +641,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         char buf[64];
         snprintf(buf, sizeof(buf), "VMDK version %d",
                  le32_to_cpu(header.version));
-        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                bs->device_name, "vmdk", buf);
+        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
+                  bs->device_name, "vmdk", buf);
         return -ENOTSUP;
     } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
         /* VMware KB 2064959 explains that version 3 added support for
@@ -653,7 +654,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     }
 
     if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
-        error_report("L2 table size too big");
+        error_setg(errp, "L2 table size too big");
         return -EINVAL;
     }
 
@@ -669,8 +670,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     }
     if (bdrv_getlength(file) <
             le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
-        error_report("File truncated, expecting at least %lld bytes",
-                le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
+        error_setg(errp, "File truncated, expecting at least %lld bytes",
+                   le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE);
         return -EINVAL;
     }
 
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4600670..3371c86 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -7,8 +7,7 @@ no file open, try 'help open'
 
 === Testing too big L2 table size ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-L2 table size too big
-qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument
+qemu-io: can't open device TEST_DIR/t.vmdk: L2 table size too big
 no file open, try 'help open'
 
 === Testing too big L1 table size ===
@@ -2045,8 +2044,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1
 
 === Testing truncated sparse ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
-qemu-img: File truncated, expecting at least 13172736 bytes
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at least 13172736 bytes
 
 === Testing version 3 ===
 image: TEST_DIR/iotest-version3.IMGFMT
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 19/20] block: do not abuse EMEDIUMTYPE

[Paolo Bonzini]

Returning "Wrong medium type" for an image that does not have a valid
header is a bit weird.  Improve the error by mentioning what format
was trying to open it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/bochs.c     | 3 ++-
 block/cow.c       | 3 ++-
 block/parallels.c | 3 ++-
 block/qcow.c      | 3 ++-
 block/qcow2.c     | 2 +-
 block/qed.c       | 3 ++-
 block/vdi.c       | 4 ++--
 block/vmdk.c      | 6 ++++--
 block/vpc.c       | 3 ++-
 9 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 51d9a90..f0f9a7e 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -129,7 +129,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         strcmp(bochs.subtype, GROWING_TYPE) ||
 	((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
 	(le32_to_cpu(bochs.version) != HEADER_V1))) {
-        return -EMEDIUMTYPE;
+        error_setg(errp, "Image not in Bochs format");
+        return -EINVAL;
     }
 
     if (le32_to_cpu(bochs.version) == HEADER_V1) {
diff --git a/block/cow.c b/block/cow.c
index 43a2150..af7b824 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -74,7 +74,8 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (be32_to_cpu(cow_header.magic) != COW_MAGIC) {
-        ret = -EMEDIUMTYPE;
+        error_setg(errp, "Image not in COW format");
+        ret = -EINVAL;
         goto fail;
     }
 
diff --git a/block/parallels.c b/block/parallels.c
index 2121e43..5c032f5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -85,7 +85,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
         (le32_to_cpu(ph.version) != HEADER_VERSION)) {
-        ret = -EMEDIUMTYPE;
+        error_setg(errp, "Image not in Parallels format");
+        ret = -EINVAL;
         goto fail;
     }
 
diff --git a/block/qcow.c b/block/qcow.c
index 23bc691..292a314 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -113,7 +113,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     be64_to_cpus(&header.l1_table_offset);
 
     if (header.magic != QCOW_MAGIC) {
-        ret = -EMEDIUMTYPE;
+        error_setg(errp, "Image not in qcow format");
+        ret = -EINVAL;
         goto fail;
     }
     if (header.version != QCOW_VERSION) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 2da62b8..fa63d37 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (header.magic != QCOW_MAGIC) {
         error_setg(errp, "Image is not in qcow2 format");
-        ret = -EMEDIUMTYPE;
+        ret = -EINVAL;
         goto fail;
     }
     if (header.version < 2 || header.version > 3) {
diff --git a/block/qed.c b/block/qed.c
index 59bdd58..7efbc3b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -391,7 +391,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     qed_header_le_to_cpu(&le_header, &s->header);
 
     if (s->header.magic != QED_MAGIC) {
-        return -EMEDIUMTYPE;
+        error_setg(errp, "Image not in QED format");
+        return -EINVAL;
     }
     if (s->header.features & ~QED_FEATURE_MASK) {
         /* image uses unsupported feature bits */
diff --git a/block/vdi.c b/block/vdi.c
index 2d7490f..68e152c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -395,8 +395,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (header.signature != VDI_SIGNATURE) {
-        logout("bad vdi signature %08x\n", header.signature);
-        ret = -EMEDIUMTYPE;
+        error_setg(errp, "Image not in VDI format (bad signature %08x)", header.signature);
+        ret = -EINVAL;
         goto fail;
     } else if (header.version != VDI_VERSION_1_1) {
         logout("unsupported version %u.%u\n",
diff --git a/block/vmdk.c b/block/vmdk.c
index f148164..888a963 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -747,7 +747,8 @@ static int vmdk_open_sparse(BlockDriverState *bs,
             return vmdk_open_vmdk4(bs, file, flags, errp);
             break;
         default:
-            return -EMEDIUMTYPE;
+            error_setg(errp, "Image not in VMDK format");
+            return -EINVAL;
             break;
     }
 }
@@ -859,7 +860,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
     BDRVVmdkState *s = bs->opaque;
 
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
-        ret = -EMEDIUMTYPE;
+        error_setg(errp, "invalid VMDK image descriptor");
+        ret = -EINVAL;
         goto exit;
     }
     if (strcmp(ct, "monolithicFlat") &&
diff --git a/block/vpc.c b/block/vpc.c
index 1d326cb..238d91a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -190,7 +190,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
         if (strncmp(footer->creator, "conectix", 8)) {
-            ret = -EMEDIUMTYPE;
+            error_setg(errp, "invalid VPC image");
+            ret = -EINVAL;
             goto fail;
         }
         disk_type = VHD_FIXED;
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 20/20] vdi: say why an image is bad

[Paolo Bonzini]

Instead of just putting it in debugging output, we can now put the
value in an Error.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vdi.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 68e152c..3859e49 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -399,39 +399,46 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto fail;
     } else if (header.version != VDI_VERSION_1_1) {
-        logout("unsupported version %u.%u\n",
-               header.version >> 16, header.version & 0xffff);
+        error_setg(errp, "unsupported VDI image (version %u.%u)",
+                   header.version >> 16, header.version & 0xffff);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.offset_bmap % SECTOR_SIZE != 0) {
         /* We only support block maps which start on a sector boundary. */
-        logout("unsupported block map offset 0x%x B\n", header.offset_bmap);
+        error_setg(errp, "unsupported VDI image (unaligned block map offset 0x%x)",
+                   header.offset_bmap);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.offset_data % SECTOR_SIZE != 0) {
         /* We only support data blocks which start on a sector boundary. */
-        logout("unsupported data offset 0x%x B\n", header.offset_data);
+        error_setg(errp, "unsupported VDI image (unaligned data offset 0x%x)",
+                   header.offset_data);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.sector_size != SECTOR_SIZE) {
-        logout("unsupported sector size %u B\n", header.sector_size);
+        error_setg(errp, "unsupported VDI image (sector size %u is not %u)",
+                   header.sector_size, SECTOR_SIZE);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.block_size != 1 * MiB) {
-        logout("unsupported block size %u B\n", header.block_size);
+        error_setg(errp, "unsupported VDI image (sector size %u is not %u)",
+                   header.block_size, 1 * MiB);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.disk_size >
                (uint64_t)header.blocks_in_image * header.block_size) {
-        logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
+        error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
+                   "image bitmap has room for %" PRIu64 ")",
+                   header.disk_size,
+                   (uint64_t)header.blocks_in_image * header.block_size);
         ret = -ENOTSUP;
         goto fail;
     } else if (!uuid_is_null(header.uuid_link)) {
-        logout("link uuid != 0, unsupported\n");
+        error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
         ret = -ENOTSUP;
         goto fail;
     } else if (!uuid_is_null(header.uuid_parent)) {
-        logout("parent uuid != 0, unsupported\n");
+        error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
         ret = -ENOTSUP;
         goto fail;
     }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 06/20] iscsi: correctly propagate errors in iscsi_open

[Stefan Hajnoczi]

On Tue, Feb 11, 2014 at 06:03:39PM +0100, Paolo Bonzini wrote:
> @@ -1349,14 +1350,15 @@ static int iscsi_reopen_prepare(BDRVReopenState *state,
>  static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
>  {
>      IscsiLun *iscsilun = bs->opaque;
> -    int ret = 0;
> +    Error *local_err = NULL;
>  
>      if (iscsilun->type != TYPE_DISK) {
>          return -ENOTSUP;
>      }
>  
> -    if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
> -        return ret;
> +    iscsi_readcapacity_sync(iscsilun, &local_err);
> +    if (local_err != NULL) {
> +        return -EIO;
>      }

Memory leak.  We need to free local_err.

Re: [Qemu-devel] [PATCH v2 00/20] Improve bdrv_open error messages

[Stefan Hajnoczi]

On Tue, Feb 11, 2014 at 06:03:33PM +0100, Paolo Bonzini wrote:
> Most of the block drivers are not using the Error** argument to
> bdrv_open, and instead just printing errors to stderr.  This series
> improves that, using Error** instead of abusing errno numbers too.
> 
> The only hurdle (caught by qemu-iotests, too) is VMDK, where we
> currently parse a file first as image, and second as a descriptor.
> This makes it hard to pick a good error message, because you do not
> know which attempt gave the most reasonable error message.  For
> this reason patches 15-17 modify this approach and push up the
> detection of vmdk image file magic numbers.
> 
> Apart from this, and from a segfault fixed by patch 7, the series
> consists of mostly trivial patches.
> 
> Paolo
> 
> Paolo Bonzini (20):
>   nbd: produce a better error if neither host nor port is passed
>   nbd: correctly propagate errors
>   nbd: inline tcp_socket_incoming_spec into sole caller
>   nbd: move socket wrappers to qemu-nbd
>   iscsi: fix indentation
>   iscsi: correctly propagate errors in iscsi_open
>   gluster: default scheme to gluster:// and host to localhost.
>   gluster: correctly propagate errors
>   cow: correctly propagate errors
>   curl: correctly propagate errors
>   qcow: correctly propagate errors
>   qed: correctly propagate errors
>   vhdx: correctly propagate errors
>   vvfat: correctly propagate errors
>   vmdk: extract vmdk_read_desc
>   vmdk: push vmdk_read_desc up to caller
>   vmdk: do not try opening a file as both image and descriptor
>   vmdk: correctly propagate errors
>   block: do not abuse EMEDIUMTYPE
>   vdi: say why an image is bad
> 
>  block/bochs.c              |   3 +-
>  block/cow.c                |  15 ++---
>  block/curl.c               |  13 ++---
>  block/gluster.c            |  28 ++++-----
>  block/iscsi.c              | 141 +++++++++++++++++++++++----------------------
>  block/nbd.c                |  43 +++++++-------
>  block/parallels.c          |   3 +-
>  block/qcow.c               |  19 +++---
>  block/qcow2.c              |   2 +-
>  block/qed.c                |  19 +++---
>  block/vdi.c                |  29 ++++++----
>  block/vhdx.c               |  21 +++----
>  block/vmdk.c               | 121 ++++++++++++++++++++++++--------------
>  block/vpc.c                |   3 +-
>  block/vvfat.c              |   9 +--
>  include/block/nbd.h        |   6 --
>  nbd.c                      |  66 ---------------------
>  qemu-nbd.c                 |  52 +++++++++++++++++
>  tests/qemu-iotests/059.out |   6 +-
>  19 files changed, 302 insertions(+), 297 deletions(-)

Looks good except for the memory leak that I commented on.

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Kevin Wolf]

Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 7fc0b12..43a2150 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
>          char version[64];
>          snprintf(version, sizeof(version),
>                 "COW version %d", cow_header.version);
> -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>              bs->device_name, "cow", version);
>          ret = -ENOTSUP;
>          goto fail;
> @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>      struct stat st;
>      int64_t image_sectors = 0;
>      const char *image_filename = NULL;
> -    Error *local_err = NULL;
>      int ret;
>      BlockDriverState *cow_bs;
>  
> @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>          options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, errp);
>      if (ret < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
>          return ret;
>      }
>  
> -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> -                         &local_err);
> +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
>      if (ret < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
>          return ret;
>      }

This is technically correct, but I think general policy is that using
the local_err pattern is preferred anyway.

Kevin

[Qemu-devel] [PATCH v3 06/20] iscsi: correctly propagate errors in iscsi_open

[Paolo Bonzini]

Before:
    $ ./qemu-io-old
    qemu-io-old> open -r -o file.driver=iscsi,file.filename=foo
    Failed to parse URL : foo
    qemu-io-old: can't open device (null): Could not open 'foo': Invalid argument

After:
    $ ./qemu-io
    qemu-io> open -r -o file.driver=iscsi,file.filename=foo
    qemu-io: can't open device (null): Failed to parse URL : foo

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v2->v3 fix memory leak reported by Stefan

 block/iscsi.c | 103 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index e654a57..a23eba3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -856,7 +856,8 @@ retry:
 
 #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
 
-static int parse_chap(struct iscsi_context *iscsi, const char *target)
+static void parse_chap(struct iscsi_context *iscsi, const char *target,
+                       Error **errp)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -865,37 +866,35 @@ static int parse_chap(struct iscsi_context *iscsi, const char *target)
 
     list = qemu_find_opts("iscsi");
     if (!list) {
-        return 0;
+        return;
     }
 
     opts = qemu_opts_find(list, target);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
-            return 0;
+            return;
         }
     }
 
     user = qemu_opt_get(opts, "user");
     if (!user) {
-        return 0;
+        return;
     }
 
     password = qemu_opt_get(opts, "password");
     if (!password) {
-        error_report("CHAP username specified but no password was given");
-        return -1;
+        error_setg(errp, "CHAP username specified but no password was given");
+        return;
     }
 
     if (iscsi_set_initiator_username_pwd(iscsi, user, password)) {
-        error_report("Failed to set initiator username and password");
-        return -1;
+        error_setg(errp, "Failed to set initiator username and password");
     }
-
-    return 0;
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
+static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+                                Error **errp)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -928,7 +927,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target)
     } else if (!strcmp(digest, "NONE-CRC32C")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
     } else {
-        error_report("Invalid header-digest setting : %s", digest);
+        error_setg(errp, "Invalid header-digest setting : %s", digest);
     }
 }
 
@@ -986,12 +985,11 @@ static void iscsi_nop_timed_event(void *opaque)
 }
 #endif
 
-static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
+static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
 {
     struct scsi_task *task = NULL;
     struct scsi_readcapacity10 *rc10 = NULL;
     struct scsi_readcapacity16 *rc16 = NULL;
-    int ret = 0;
     int retries = ISCSI_CMD_RETRIES; 
 
     do {
@@ -1006,8 +1004,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             if (task != NULL && task->status == SCSI_STATUS_GOOD) {
                 rc16 = scsi_datain_unmarshall(task);
                 if (rc16 == NULL) {
-                    error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
-                    ret = -EINVAL;
+                    error_setg(errp, "iSCSI: Failed to unmarshall readcapacity16 data.");
                 } else {
                     iscsilun->block_size = rc16->block_length;
                     iscsilun->num_blocks = rc16->returned_lba + 1;
@@ -1021,8 +1018,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             if (task != NULL && task->status == SCSI_STATUS_GOOD) {
                 rc10 = scsi_datain_unmarshall(task);
                 if (rc10 == NULL) {
-                    error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
-                    ret = -EINVAL;
+                    error_setg(errp, "iSCSI: Failed to unmarshall readcapacity10 data.");
                 } else {
                     iscsilun->block_size = rc10->block_size;
                     if (rc10->lba == 0) {
@@ -1035,20 +1031,18 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
             }
             break;
         default:
-            return 0;
+            return;
         }
     } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
              && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
              && retries-- > 0);
 
     if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-        error_report("iSCSI: failed to send readcapacity10 command.");
-        ret = -EINVAL;
+        error_setg(errp, "iSCSI: failed to send readcapacity10 command.");
     }
     if (task) {
         scsi_free_scsi_task(task);
     }
-    return ret;
 }
 
 /* TODO Convert to fine grained options */
@@ -1066,7 +1060,7 @@ static QemuOptsList runtime_opts = {
 };
 
 static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
-                                          int evpd, int pc)
+                                          int evpd, int pc, Error **errp)
 {
     int full_size;
     struct scsi_task *task = NULL;
@@ -1088,8 +1082,8 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun,
     return task;
 
 fail:
-    error_report("iSCSI: Inquiry command failed : %s",
-                 iscsi_get_error(iscsi));
+    error_setg(errp, "iSCSI: Inquiry command failed : %s",
+               iscsi_get_error(iscsi));
     if (task) {
         scsi_free_scsi_task(task);
         return NULL;
@@ -1116,27 +1110,25 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
     if ((BDRV_SECTOR_SIZE % 512) != 0) {
-        error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
-                     "BDRV_SECTOR_SIZE(%lld) is not a multiple "
-                     "of 512", BDRV_SECTOR_SIZE);
+        error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
+                   "BDRV_SECTOR_SIZE(%lld) is not a multiple "
+                   "of 512", BDRV_SECTOR_SIZE);
         return -EINVAL;
     }
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     filename = qemu_opt_get(opts, "filename");
 
-
     iscsi_url = iscsi_parse_full_url(iscsi, filename);
     if (iscsi_url == NULL) {
-        error_report("Failed to parse URL : %s", filename);
+        error_setg(errp, "Failed to parse URL : %s", filename);
         ret = -EINVAL;
         goto out;
     }
@@ -1147,13 +1139,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
-        error_report("iSCSI: Failed to create iSCSI context.");
+        error_setg(errp, "iSCSI: Failed to create iSCSI context.");
         ret = -ENOMEM;
         goto out;
     }
 
     if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
-        error_report("iSCSI: Failed to set target name.");
+        error_setg(errp, "iSCSI: Failed to set target name.");
         ret = -EINVAL;
         goto out;
     }
@@ -1162,21 +1154,22 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user,
                                               iscsi_url->passwd);
         if (ret != 0) {
-            error_report("Failed to set initiator username and password");
+            error_setg(errp, "Failed to set initiator username and password");
             ret = -EINVAL;
             goto out;
         }
     }
 
     /* check if we got CHAP username/password via the options */
-    if (parse_chap(iscsi, iscsi_url->target) != 0) {
-        error_report("iSCSI: Failed to set CHAP user/password");
+    parse_chap(iscsi, iscsi_url->target, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
-        error_report("iSCSI: Failed to set session type to normal.");
+        error_setg(errp, "iSCSI: Failed to set session type to normal.");
         ret = -EINVAL;
         goto out;
     }
@@ -1184,10 +1177,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
 
     /* check if we got HEADER_DIGEST via the options */
-    parse_header_digest(iscsi, iscsi_url->target);
+    parse_header_digest(iscsi, iscsi_url->target, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto out;
+    }
 
     if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
-        error_report("iSCSI: Failed to connect to LUN : %s",
+        error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
             iscsi_get_error(iscsi));
         ret = -EINVAL;
         goto out;
@@ -1199,14 +1197,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 0, 0, 36);
 
     if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-        error_report("iSCSI: failed to send inquiry command.");
+        error_setg(errp, "iSCSI: failed to send inquiry command.");
         ret = -EINVAL;
         goto out;
     }
 
     inq = scsi_datain_unmarshall(task);
     if (inq == NULL) {
-        error_report("iSCSI: Failed to unmarshall inquiry data.");
+        error_setg(errp, "iSCSI: Failed to unmarshall inquiry data.");
         ret = -EINVAL;
         goto out;
     }
@@ -1214,7 +1212,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsilun->type = inq->periperal_device_type;
     iscsilun->has_write_same = true;
 
-    if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
+    iscsi_readcapacity_sync(iscsilun, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
         goto out;
     }
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
@@ -1232,14 +1232,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     if (iscsilun->lbpme) {
         struct scsi_inquiry_logical_block_provisioning *inq_lbp;
         task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
-                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
+                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
+                                errp);
         if (task == NULL) {
             ret = -EINVAL;
             goto out;
         }
         inq_lbp = scsi_datain_unmarshall(task);
         if (inq_lbp == NULL) {
-            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
             ret = -EINVAL;
             goto out;
         }
@@ -1252,14 +1253,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
         struct scsi_inquiry_block_limits *inq_bl;
         task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
-                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS, errp);
         if (task == NULL) {
             ret = -EINVAL;
             goto out;
         }
         inq_bl = scsi_datain_unmarshall(task);
         if (inq_bl == NULL) {
-            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob");
             ret = -EINVAL;
             goto out;
         }
@@ -1349,14 +1350,16 @@ static int iscsi_reopen_prepare(BDRVReopenState *state,
 static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 {
     IscsiLun *iscsilun = bs->opaque;
-    int ret = 0;
+    Error *local_err = NULL;
 
     if (iscsilun->type != TYPE_DISK) {
         return -ENOTSUP;
     }
 
-    if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
-        return ret;
+    iscsi_readcapacity_sync(iscsilun, &local_err);
+    if (local_err != NULL) {
+        error_free(local_err);
+        return -EIO;
     }
 
     if (offset > iscsi_getlength(bs)) {
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 00/20] Improve bdrv_open error messages

[Paolo Bonzini]

Il 14/02/2014 17:31, Stefan Hajnoczi ha scritto:
>> >  19 files changed, 302 insertions(+), 297 deletions(-)
> Looks good except for the memory leak that I commented on.
>
>

Thanks!  I sent v3 of that one patch.

Paolo

Re: [Qemu-devel] [PATCH v2 01/20] nbd: produce a better error if neither host nor port is passed

[Jeff Cody]

On Tue, Feb 11, 2014 at 06:03:34PM +0100, Paolo Bonzini wrote:
> Before:
>     $ qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
>     $ ./qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     path and host may not be used at the same time.
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
> 
> After:
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd
>     one of path and host must be specified.
>     qemu-io: can't open device (null): Could not open image: Invalid argument
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     path and host may not be used at the same time.
>     qemu-io: can't open device (null): Could not open image: Invalid argument
>

This breaks test 051, which is parsing the error output.  Could you
also update 051.out in this patch?  This should fix it:


diff --git tests/qemu-iotests/051.out tests/qemu-iotests/051.out
index 30e2dbd..7de1870 100644
--- tests/qemu-iotests/051.out
+++ tests/qemu-iotests/051.out
@@ -231,7 +231,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
+QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level
@@ -240,7 +240,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
+QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level


> Next patch will fix the error propagation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nbd.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 327e913..fd89083 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>  {
>      Error *local_err = NULL;
>  
> -    if (qdict_haskey(options, "path")) {
> -        if (qdict_haskey(options, "host")) {
> +    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> +        if (qdict_haskey(options, "path")) {
>              qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
>                            "be used at the same time.");
> -            return -EINVAL;
> +        } else {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> +                          "must be specified.");
>          }
> -        s->client.is_unix = true;
> -    } else if (qdict_haskey(options, "host")) {
> -        s->client.is_unix = false;
> -    } else {
>          return -EINVAL;
>      }
>  
> +    s->client.is_unix = qdict_haskey(options, "path");
>      s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
>                                        &error_abort);
>  
> -- 
> 1.8.5.3
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Jeff Cody]

On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block/cow.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block/cow.c b/block/cow.c
> > index 7fc0b12..43a2150 100644
> > --- a/block/cow.c
> > +++ b/block/cow.c
> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
> >          char version[64];
> >          snprintf(version, sizeof(version),
> >                 "COW version %d", cow_header.version);
> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> >              bs->device_name, "cow", version);
> >          ret = -ENOTSUP;
> >          goto fail;
> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> >      struct stat st;
> >      int64_t image_sectors = 0;
> >      const char *image_filename = NULL;
> > -    Error *local_err = NULL;
> >      int ret;
> >      BlockDriverState *cow_bs;
> >  
> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> >          options++;
> >      }
> >  
> > -    ret = bdrv_create_file(filename, options, &local_err);
> > +    ret = bdrv_create_file(filename, options, errp);
> >      if (ret < 0) {
> > -        qerror_report_err(local_err);
> > -        error_free(local_err);
> >          return ret;
> >      }
> >  
> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> > -                         &local_err);
> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
> >      if (ret < 0) {
> > -        qerror_report_err(local_err);
> > -        error_free(local_err);
> >          return ret;
> >      }
> 
> This is technically correct, but I think general policy is that using
> the local_err pattern is preferred anyway.
>

If I recall correct, I think there are several places that pass errp
along.  How about this for a rule of thumb policy: use the local_err
method if the function does not indicate error outside of the passed
Error pointer.

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Paolo Bonzini]

Il 14/02/2014 17:45, Kevin Wolf ha scritto:
>> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> > -                         &local_err);
>> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
>> >      if (ret < 0) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> >          return ret;
>> >      }
> This is technically correct, but I think general policy is that using
> the local_err pattern is preferred anyway.

I think only for void-returning functions.  I checked callers of 
monitor_get_fd and they use this pattern, even if they pass &local_err 
instead of errp.

Paolo

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Kevin Wolf]

Am 14.02.2014 um 18:02 hat Paolo Bonzini geschrieben:
> Il 14/02/2014 17:45, Kevin Wolf ha scritto:
> >>> -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> >>> -                         &local_err);
> >>> +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
> >>>      if (ret < 0) {
> >>> -        qerror_report_err(local_err);
> >>> -        error_free(local_err);
> >>>          return ret;
> >>>      }
> >This is technically correct, but I think general policy is that using
> >the local_err pattern is preferred anyway.
> 
> I think only for void-returning functions.  I checked callers of
> monitor_get_fd and they use this pattern, even if they pass
> &local_err instead of errp.

Eventually this function will return void; having both a -errno return
and the errp argument is just an intermediate step (as probably in all
other cases). So I still think this is going in the wrong direction and
will make the conversion harder than necessary. Now that this patch
series is already here, I won't insist on respinning it, but please
be aware that you're just creating additional work for other people and
keep existing local_errs in any future patches.

Kevin

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Paolo Bonzini]

Il 14/02/2014 19:19, Kevin Wolf ha scritto:
> Eventually this function will return void; having both a -errno return
> and the errp argument is just an intermediate step (as probably in all
> other cases). So I still think this is going in the wrong direction and
> will make the conversion harder than necessary. Now that this patch
> series is already here, I won't insist on respinning it, but please
> be aware that you're just creating additional work for other people and
> keep existing local_errs in any future patches.

Ok, this makes sense, but it is an exception to the general policy. :)

I'll respin v3 and test NBD too.

Paolo

Re: [Qemu-devel] [PATCH v2 01/20] nbd: produce a better error if neither host nor port is passed

[Jeff Cody]

On Fri, Feb 14, 2014 at 11:54:34AM -0500, Jeff Cody wrote:
> On Tue, Feb 11, 2014 at 06:03:34PM +0100, Paolo Bonzini wrote:
> > Before:
> >     $ qemu-io-old
> >     qemu-io-old> open -r -o file.driver=nbd
> >     qemu-io-old: can't open device (null): Could not open image: Invalid argument
> >     $ ./qemu-io-old
> >     qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> >     path and host may not be used at the same time.
> >     qemu-io-old: can't open device (null): Could not open image: Invalid argument
> > 
> > After:
> >     $ ./qemu-io
> >     qemu-io> open -r -o file.driver=nbd
> >     one of path and host must be specified.
> >     qemu-io: can't open device (null): Could not open image: Invalid argument
> >     $ ./qemu-io
> >     qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
> >     path and host may not be used at the same time.
> >     qemu-io: can't open device (null): Could not open image: Invalid argument
> >
> 
> This breaks test 051, which is parsing the error output.  Could you
> also update 051.out in this patch?  This should fix it:
> 
> 
> diff --git tests/qemu-iotests/051.out tests/qemu-iotests/051.out
> index 30e2dbd..7de1870 100644
> --- tests/qemu-iotests/051.out
> +++ tests/qemu-iotests/051.out
> @@ -231,7 +231,7 @@ Testing: -drive driver=file
>  QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
>  
>  Testing: -drive driver=nbd
> -QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
> +QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
>  
>  Testing: -drive driver=raw
>  QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level
> @@ -240,7 +240,7 @@ Testing: -drive file.driver=file
>  QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
>  
>  Testing: -drive file.driver=nbd
> -QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
> +QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
>  
>  Testing: -drive file.driver=raw
>  QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level
>

Sorry Paolo, the above would be if patches 1 and 2 were squashed together. 

This would be the diff for 051.out for just this patch:

diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 30e2dbd..3e8d962 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -231,6 +231,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
+QEMU_PROG: -drive driver=nbd: one of path and host must be specified.
 QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
 
 Testing: -drive driver=raw
@@ -240,6 +241,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
+QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified.
 QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
 
 Testing: -drive file.driver=raw


> 
> > Next patch will fix the error propagation.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block/nbd.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 327e913..fd89083 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> >  {
> >      Error *local_err = NULL;
> >  
> > -    if (qdict_haskey(options, "path")) {
> > -        if (qdict_haskey(options, "host")) {
> > +    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> > +        if (qdict_haskey(options, "path")) {
> >              qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
> >                            "be used at the same time.");
> > -            return -EINVAL;
> > +        } else {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> > +                          "must be specified.");
> >          }
> > -        s->client.is_unix = true;
> > -    } else if (qdict_haskey(options, "host")) {
> > -        s->client.is_unix = false;
> > -    } else {
> >          return -EINVAL;
> >      }
> >  
> > +    s->client.is_unix = qdict_haskey(options, "path");
> >      s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
> >                                        &error_abort);
> >  
> > -- 
> > 1.8.5.3
> > 
> > 
> > 
> 

Re: [Qemu-devel] [PATCH v2 02/20] nbd: correctly propagate errors

[Jeff Cody]

On Tue, Feb 11, 2014 at 06:03:35PM +0100, Paolo Bonzini wrote:
> Before:
>     $ ./qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd
>     one of path and host must be specified.
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
>     $ ./qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     path and host may not be used at the same time.
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
> 
> After:
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd
>     qemu-io: can't open device (null): one of path and host must be specified.
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     qemu-io: can't open device (null): path and host may not be used at the same time.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


And this is the diff for 051.out for this patch (patch 2):


diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 3e8d962..7de1870 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -231,8 +231,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified.
-QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
+QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level
@@ -241,8 +240,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified.
-QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
+QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level



> ---
>  block/nbd.c         | 34 ++++++++++++++++------------------
>  include/block/nbd.h |  1 -
>  nbd.c               | 12 ------------
>  3 files changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index fd89083..69f336b 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -188,19 +188,18 @@ out:
>      g_free(file);
>  }
>  
> -static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> +static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
> +                       Error **errp)
>  {
>      Error *local_err = NULL;
>  
>      if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
>          if (qdict_haskey(options, "path")) {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
> -                          "be used at the same time.");
> +            error_setg(errp, "path and host may not be used at the same time.");
>          } else {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> -                          "must be specified.");
> +            error_setg(errp, "one of path and host must be specified.");
>          }
> -        return -EINVAL;
> +        return;
>      }
>  
>      s->client.is_unix = qdict_haskey(options, "path");
> @@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>  
>      qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return -EINVAL;
> +        error_propagate(errp, local_err);
> +        return;
>      }
>  
>      if (!qemu_opt_get(s->socket_opts, "port")) {
> @@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>      if (*export) {
>          qdict_del(options, "export");
>      }
> -
> -    return 0;
>  }
>  
> -static int nbd_establish_connection(BlockDriverState *bs)
> +static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
>  {
>      BDRVNBDState *s = bs->opaque;
>      int sock;
>  
>      if (s->client.is_unix) {
> -        sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
> +        sock = unix_connect_opts(s->socket_opts, errp, NULL, NULL);
>      } else {
> -        sock = tcp_socket_outgoing_opts(s->socket_opts);
> +        sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
>          if (sock >= 0) {
>              socket_set_nodelay(sock);
>          }
> @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>      BDRVNBDState *s = bs->opaque;
>      char *export = NULL;
>      int result, sock;
> +    Error *local_err = NULL;
>  
>      /* Pop the config into our state object. Exit if invalid. */
> -    result = nbd_config(s, options, &export);
> -    if (result != 0) {
> -        return result;
> +    nbd_config(s, options, &export, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
>      }
>  
>      /* establish TCP connection, return error if it fails
>       * TODO: Configurable retry-until-timeout behaviour.
>       */
> -    sock = nbd_establish_connection(bs);
> +    sock = nbd_establish_connection(bs, errp);
>      if (sock < 0) {
>          return sock;
>      }
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index c90f5e4..e10ab82 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -64,7 +64,6 @@ enum {
>  ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
>  int tcp_socket_incoming(const char *address, uint16_t port);
>  int tcp_socket_incoming_spec(const char *address_and_port);
> -int tcp_socket_outgoing_opts(QemuOpts *opts);
>  int unix_socket_outgoing(const char *path);
>  int unix_socket_incoming(const char *path);
>  
> diff --git a/nbd.c b/nbd.c
> index 030f56b..17ca95b 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -199,18 +199,6 @@ static void combine_addr(char *buf, size_t len, const char* address,
>      }
>  }
>  
> -int tcp_socket_outgoing_opts(QemuOpts *opts)
> -{
> -    Error *local_err = NULL;
> -    int fd = inet_connect_opts(opts, &local_err, NULL, NULL);
> -    if (local_err != NULL) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -    }
> -
> -    return fd;
> -}
> -
>  int tcp_socket_incoming(const char *address, uint16_t port)
>  {
>      char address_and_port[128];
> -- 
> 1.8.5.3
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
>> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> > ---
>> >  block/cow.c | 12 +++---------
>> >  1 file changed, 3 insertions(+), 9 deletions(-)
>> > 
>> > diff --git a/block/cow.c b/block/cow.c
>> > index 7fc0b12..43a2150 100644
>> > --- a/block/cow.c
>> > +++ b/block/cow.c
>> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
>> >          char version[64];
>> >          snprintf(version, sizeof(version),
>> >                 "COW version %d", cow_header.version);
>> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> >              bs->device_name, "cow", version);
>> >          ret = -ENOTSUP;
>> >          goto fail;
>> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>> >      struct stat st;
>> >      int64_t image_sectors = 0;
>> >      const char *image_filename = NULL;
>> > -    Error *local_err = NULL;
>> >      int ret;
>> >      BlockDriverState *cow_bs;
>> >  
>> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>> >          options++;
>> >      }
>> >  
>> > -    ret = bdrv_create_file(filename, options, &local_err);
>> > +    ret = bdrv_create_file(filename, options, errp);
>> >      if (ret < 0) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> >          return ret;
>> >      }
>> >  
>> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> > -                         &local_err);
>> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
>> >      if (ret < 0) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> >          return ret;
>> >      }
>> 
>> This is technically correct, but I think general policy is that using
>> the local_err pattern is preferred anyway.
>>
>
> If I recall correct, I think there are several places that pass errp
> along.  How about this for a rule of thumb policy: use the local_err
> method if the function does not indicate error outside of the passed
> Error pointer.

Use &local_err when you need to examine the error object.  Passing errp
directly is no good then, because it may be null.

When you're forwarding errors without examining them, then passing errp
directly is just fine.

Re: [Qemu-devel] [PATCH v2 01/20] nbd: produce a better error if neither host nor port is passed

[Paolo Bonzini]

Il 14/02/2014 17:54, Jeff Cody ha scritto:
> On Tue, Feb 11, 2014 at 06:03:34PM +0100, Paolo Bonzini wrote:
>> Before:
>>     $ qemu-io-old
>>     qemu-io-old> open -r -o file.driver=nbd
>>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
>>     $ ./qemu-io-old
>>     qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>>     path and host may not be used at the same time.
>>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
>>
>> After:
>>     $ ./qemu-io
>>     qemu-io> open -r -o file.driver=nbd
>>     one of path and host must be specified.
>>     qemu-io: can't open device (null): Could not open image: Invalid argument
>>     $ ./qemu-io
>>     qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>>     path and host may not be used at the same time.
>>     qemu-io: can't open device (null): Could not open image: Invalid argument
>>
>
> This breaks test 051, which is parsing the error output.  Could you
> also update 051.out in this patch?  This should fix it:
>
>
> diff --git tests/qemu-iotests/051.out tests/qemu-iotests/051.out
> index 30e2dbd..7de1870 100644
> --- tests/qemu-iotests/051.out
> +++ tests/qemu-iotests/051.out
> @@ -231,7 +231,7 @@ Testing: -drive driver=file
>  QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
>
>  Testing: -drive driver=nbd
> -QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
> +QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
>
>  Testing: -drive driver=raw
>  QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level
> @@ -240,7 +240,7 @@ Testing: -drive file.driver=file
>  QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name
>
>  Testing: -drive file.driver=nbd
> -QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument
> +QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified.
>
>  Testing: -drive file.driver=raw
>  QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level

Thanks, I was working on an older checkout.  Fixed and tested now.

Paolo

>
>> Next patch will fix the error propagation.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/nbd.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 327e913..fd89083 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>>  {
>>      Error *local_err = NULL;
>>
>> -    if (qdict_haskey(options, "path")) {
>> -        if (qdict_haskey(options, "host")) {
>> +    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
>> +        if (qdict_haskey(options, "path")) {
>>              qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
>>                            "be used at the same time.");
>> -            return -EINVAL;
>> +        } else {
>> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
>> +                          "must be specified.");
>>          }
>> -        s->client.is_unix = true;
>> -    } else if (qdict_haskey(options, "host")) {
>> -        s->client.is_unix = false;
>> -    } else {
>>          return -EINVAL;
>>      }
>>
>> +    s->client.is_unix = qdict_haskey(options, "path");
>>      s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
>>                                        &error_abort);
>>
>> --
>> 1.8.5.3
>>
>>
>>
>
>

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Fam Zheng]

On Sat, 02/15 11:01, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
> >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> > ---
> >> >  block/cow.c | 12 +++---------
> >> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >> > 
> >> > diff --git a/block/cow.c b/block/cow.c
> >> > index 7fc0b12..43a2150 100644
> >> > --- a/block/cow.c
> >> > +++ b/block/cow.c
> >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
> >> >          char version[64];
> >> >          snprintf(version, sizeof(version),
> >> >                 "COW version %d", cow_header.version);
> >> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> >> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> >> >              bs->device_name, "cow", version);
> >> >          ret = -ENOTSUP;
> >> >          goto fail;
> >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> >> >      struct stat st;
> >> >      int64_t image_sectors = 0;
> >> >      const char *image_filename = NULL;
> >> > -    Error *local_err = NULL;
> >> >      int ret;
> >> >      BlockDriverState *cow_bs;
> >> >  
> >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> >> >          options++;
> >> >      }
> >> >  
> >> > -    ret = bdrv_create_file(filename, options, &local_err);
> >> > +    ret = bdrv_create_file(filename, options, errp);
> >> >      if (ret < 0) {
> >> > -        qerror_report_err(local_err);
> >> > -        error_free(local_err);
> >> >          return ret;
> >> >      }
> >> >  
> >> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> >> > -                         &local_err);
> >> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
> >> >      if (ret < 0) {
> >> > -        qerror_report_err(local_err);
> >> > -        error_free(local_err);
> >> >          return ret;
> >> >      }
> >> 
> >> This is technically correct, but I think general policy is that using
> >> the local_err pattern is preferred anyway.
> >>
> >
> > If I recall correct, I think there are several places that pass errp
> > along.  How about this for a rule of thumb policy: use the local_err
> > method if the function does not indicate error outside of the passed
> > Error pointer.
> 
> Use &local_err when you need to examine the error object.  Passing errp
> directly is no good then, because it may be null.
> 
> When you're forwarding errors without examining them, then passing errp
> directly is just fine.
> 

Does this mean that error_is_set() is always used by programmer to check a
non-NULL error pointer? Is there any case to call error_is_set(errp) without
knowing if errp is NULL or not? If no, should we enforce the rule and add
assert(errp) in error_is_set()?

Thanks,
Fam

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Paolo Bonzini]

Il 17/02/2014 14:15, Fam Zheng ha scritto:
> Does this mean that error_is_set() is always used by programmer to check a
> non-NULL error pointer? Is there any case to call error_is_set(errp) without
> knowing if errp is NULL or not? If no, should we enforce the rule and add
> assert(errp) in error_is_set()?

I think we shouldn't need error_is_set() at all...

Paolo

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Jeff Cody]

On Mon, Feb 17, 2014 at 02:20:10PM +0100, Paolo Bonzini wrote:
> Il 17/02/2014 14:15, Fam Zheng ha scritto:
> >Does this mean that error_is_set() is always used by programmer to check a
> >non-NULL error pointer? Is there any case to call error_is_set(errp) without
> >knowing if errp is NULL or not? If no, should we enforce the rule and add
> >assert(errp) in error_is_set()?
> 
> I think we shouldn't need error_is_set() at all...
>

By this do you mean the caller should dereference errp explicitly to
check to see if an error is set, or that there should not be void
functions that only indicate error via errp?

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Kevin Wolf]

Am 17.02.2014 um 14:15 hat Fam Zheng geschrieben:
> On Sat, 02/15 11:01, Markus Armbruster wrote:
> > Jeff Cody <jcody@redhat.com> writes:
> > 
> > > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
> > >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
> > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> > ---
> > >> >  block/cow.c | 12 +++---------
> > >> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > >> > 
> > >> > diff --git a/block/cow.c b/block/cow.c
> > >> > index 7fc0b12..43a2150 100644
> > >> > --- a/block/cow.c
> > >> > +++ b/block/cow.c
> > >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
> > >> >          char version[64];
> > >> >          snprintf(version, sizeof(version),
> > >> >                 "COW version %d", cow_header.version);
> > >> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> > >> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> > >> >              bs->device_name, "cow", version);
> > >> >          ret = -ENOTSUP;
> > >> >          goto fail;
> > >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> > >> >      struct stat st;
> > >> >      int64_t image_sectors = 0;
> > >> >      const char *image_filename = NULL;
> > >> > -    Error *local_err = NULL;
> > >> >      int ret;
> > >> >      BlockDriverState *cow_bs;
> > >> >  
> > >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
> > >> >          options++;
> > >> >      }
> > >> >  
> > >> > -    ret = bdrv_create_file(filename, options, &local_err);
> > >> > +    ret = bdrv_create_file(filename, options, errp);
> > >> >      if (ret < 0) {
> > >> > -        qerror_report_err(local_err);
> > >> > -        error_free(local_err);
> > >> >          return ret;
> > >> >      }
> > >> >  
> > >> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> > >> > -                         &local_err);
> > >> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
> > >> >      if (ret < 0) {
> > >> > -        qerror_report_err(local_err);
> > >> > -        error_free(local_err);
> > >> >          return ret;
> > >> >      }
> > >> 
> > >> This is technically correct, but I think general policy is that using
> > >> the local_err pattern is preferred anyway.
> > >>
> > >
> > > If I recall correct, I think there are several places that pass errp
> > > along.  How about this for a rule of thumb policy: use the local_err
> > > method if the function does not indicate error outside of the passed
> > > Error pointer.
> > 
> > Use &local_err when you need to examine the error object.  Passing errp
> > directly is no good then, because it may be null.
> > 
> > When you're forwarding errors without examining them, then passing errp
> > directly is just fine.
> > 
> 
> Does this mean that error_is_set() is always used by programmer to check a
> non-NULL error pointer? Is there any case to call error_is_set(errp) without
> knowing if errp is NULL or not? If no, should we enforce the rule and add
> assert(errp) in error_is_set()?

Sounds like a good idea to me, it would catch bugs where you forget to
use a local_err. Of course, it requires that error_is_set() is used
instead of just using errp as a boolean, but such an assertion could
actually be a reason to make this the policy.

Kevin

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Sat, 02/15 11:01, Markus Armbruster wrote:
>> Jeff Cody <jcody@redhat.com> writes:
>> 
>> > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
>> >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
>> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> > ---
>> >> >  block/cow.c | 12 +++---------
>> >> >  1 file changed, 3 insertions(+), 9 deletions(-)
>> >> > 
>> >> > diff --git a/block/cow.c b/block/cow.c
>> >> > index 7fc0b12..43a2150 100644
>> >> > --- a/block/cow.c
>> >> > +++ b/block/cow.c
>> >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
>> >> >          char version[64];
>> >> >          snprintf(version, sizeof(version),
>> >> >                 "COW version %d", cow_header.version);
>> >> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> >> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> >> >              bs->device_name, "cow", version);
>> >> >          ret = -ENOTSUP;
>> >> >          goto fail;
>> >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>> >> >      struct stat st;
>> >> >      int64_t image_sectors = 0;
>> >> >      const char *image_filename = NULL;
>> >> > -    Error *local_err = NULL;
>> >> >      int ret;
>> >> >      BlockDriverState *cow_bs;
>> >> >  
>> >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>> >> >          options++;
>> >> >      }
>> >> >  
>> >> > -    ret = bdrv_create_file(filename, options, &local_err);
>> >> > +    ret = bdrv_create_file(filename, options, errp);
>> >> >      if (ret < 0) {
>> >> > -        qerror_report_err(local_err);
>> >> > -        error_free(local_err);
>> >> >          return ret;
>> >> >      }
>> >> >  
>> >> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> >> > -                         &local_err);
>> >> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp);
>> >> >      if (ret < 0) {
>> >> > -        qerror_report_err(local_err);
>> >> > -        error_free(local_err);
>> >> >          return ret;
>> >> >      }
>> >> 
>> >> This is technically correct, but I think general policy is that using
>> >> the local_err pattern is preferred anyway.
>> >>
>> >
>> > If I recall correct, I think there are several places that pass errp
>> > along.  How about this for a rule of thumb policy: use the local_err
>> > method if the function does not indicate error outside of the passed
>> > Error pointer.
>> 
>> Use &local_err when you need to examine the error object.  Passing errp
>> directly is no good then, because it may be null.
>> 
>> When you're forwarding errors without examining them, then passing errp
>> directly is just fine.
>> 
>
> Does this mean that error_is_set() is always used by programmer to check a
> non-NULL error pointer? Is there any case to call error_is_set(errp) without
> knowing if errp is NULL or not? If no, should we enforce the rule and add
> assert(errp) in error_is_set()?

If you know ERRP can't be null, then error_is_set(ERRP) is pointless.
Just test *ERRP instead.

If ERRP may be null, then error_is_set(ERRP) makes some sense: it saves
you spelling out ERRP && *ERRP.  Personally, I'd prefer it spelled out,
though.

Note that testing an Error ** that may be null to answer the question
"did this fail" is *wrong*, regardless of whether you use error_is_set()
or spell it out.

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Fam Zheng]

On Mon, 02/17 14:20, Paolo Bonzini wrote:
> Il 17/02/2014 14:15, Fam Zheng ha scritto:
> >Does this mean that error_is_set() is always used by programmer to check a
> >non-NULL error pointer? Is there any case to call error_is_set(errp) without
> >knowing if errp is NULL or not? If no, should we enforce the rule and add
> >assert(errp) in error_is_set()?
> 
> I think we shouldn't need error_is_set() at all...
> 

Thinking about both use cases (errp is NULL or not) and I agree to this.

Fam

Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Fam Zheng]

On Mon, 02/17 15:59, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> > On Sat, 02/15 11:01, Markus Armbruster wrote:
> > Does this mean that error_is_set() is always used by programmer to check a
> > non-NULL error pointer? Is there any case to call error_is_set(errp) without
> > knowing if errp is NULL or not? If no, should we enforce the rule and add
> > assert(errp) in error_is_set()?
> 
> If you know ERRP can't be null, then error_is_set(ERRP) is pointless.
> Just test *ERRP instead.
> 
> If ERRP may be null, then error_is_set(ERRP) makes some sense: it saves
> you spelling out ERRP && *ERRP.  Personally, I'd prefer it spelled out,
> though.

So the question is whether the returned boolean has any value to anyboday, if
ERRP may be NULL.

Fam

Re: [Qemu-devel] [PATCH v2 19/20] block: do not abuse EMEDIUMTYPE

[Fam Zheng]

On Tue, 02/11 18:03, Paolo Bonzini wrote:
> Returning "Wrong medium type" for an image that does not have a valid
> header is a bit weird.  Improve the error by mentioning what format
> was trying to open it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/bochs.c     | 3 ++-
>  block/cow.c       | 3 ++-
>  block/parallels.c | 3 ++-
>  block/qcow.c      | 3 ++-
>  block/qcow2.c     | 2 +-
>  block/qed.c       | 3 ++-
>  block/vdi.c       | 4 ++--
>  block/vmdk.c      | 6 ++++--
>  block/vpc.c       | 3 ++-
>  9 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/block/bochs.c b/block/bochs.c
> index 51d9a90..f0f9a7e 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -129,7 +129,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>          strcmp(bochs.subtype, GROWING_TYPE) ||
>  	((le32_to_cpu(bochs.version) != HEADER_VERSION) &&
>  	(le32_to_cpu(bochs.version) != HEADER_V1))) {
> -        return -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in Bochs format");
> +        return -EINVAL;
>      }
>  
>      if (le32_to_cpu(bochs.version) == HEADER_V1) {
> diff --git a/block/cow.c b/block/cow.c
> index 43a2150..af7b824 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -74,7 +74,8 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      if (be32_to_cpu(cow_header.magic) != COW_MAGIC) {
> -        ret = -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in COW format");
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> diff --git a/block/parallels.c b/block/parallels.c
> index 2121e43..5c032f5 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -85,7 +85,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
>          (le32_to_cpu(ph.version) != HEADER_VERSION)) {
> -        ret = -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in Parallels format");
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> diff --git a/block/qcow.c b/block/qcow.c
> index 23bc691..292a314 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -113,7 +113,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>      be64_to_cpus(&header.l1_table_offset);
>  
>      if (header.magic != QCOW_MAGIC) {
> -        ret = -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in qcow format");
> +        ret = -EINVAL;
>          goto fail;
>      }
>      if (header.version != QCOW_VERSION) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2da62b8..fa63d37 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      if (header.magic != QCOW_MAGIC) {
>          error_setg(errp, "Image is not in qcow2 format");
> -        ret = -EMEDIUMTYPE;
> +        ret = -EINVAL;
>          goto fail;
>      }
>      if (header.version < 2 || header.version > 3) {
> diff --git a/block/qed.c b/block/qed.c
> index 59bdd58..7efbc3b 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -391,7 +391,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>      qed_header_le_to_cpu(&le_header, &s->header);
>  
>      if (s->header.magic != QED_MAGIC) {
> -        return -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in QED format");
> +        return -EINVAL;
>      }
>      if (s->header.features & ~QED_FEATURE_MASK) {
>          /* image uses unsupported feature bits */
> diff --git a/block/vdi.c b/block/vdi.c
> index 2d7490f..68e152c 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -395,8 +395,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      if (header.signature != VDI_SIGNATURE) {
> -        logout("bad vdi signature %08x\n", header.signature);
> -        ret = -EMEDIUMTYPE;
> +        error_setg(errp, "Image not in VDI format (bad signature %08x)", header.signature);
> +        ret = -EINVAL;
>          goto fail;
>      } else if (header.version != VDI_VERSION_1_1) {
>          logout("unsupported version %u.%u\n",
> diff --git a/block/vmdk.c b/block/vmdk.c
> index f148164..888a963 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -747,7 +747,8 @@ static int vmdk_open_sparse(BlockDriverState *bs,
>              return vmdk_open_vmdk4(bs, file, flags, errp);
>              break;
>          default:
> -            return -EMEDIUMTYPE;
> +            error_setg(errp, "Image not in VMDK format");
> +            return -EINVAL;
>              break;
>      }
>  }
> @@ -859,7 +860,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
>      BDRVVmdkState *s = bs->opaque;
>  
>      if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> -        ret = -EMEDIUMTYPE;
> +        error_setg(errp, "invalid VMDK image descriptor");
> +        ret = -EINVAL;
>          goto exit;
>      }
>      if (strcmp(ct, "monolithicFlat") &&
> diff --git a/block/vpc.c b/block/vpc.c
> index 1d326cb..238d91a 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -190,7 +190,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>              goto fail;
>          }
>          if (strncmp(footer->creator, "conectix", 8)) {
> -            ret = -EMEDIUMTYPE;
> +            error_setg(errp, "invalid VPC image");

This is also the magic check but not following the "Image not in FOO format"
pattern in this patch. But it's not a stopper.

Fam