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

[Qemu-devel] [PATCH v3 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,
and as a consequence it also avoids abuse of errno numbers.

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

        v2->v3:
                Rebase for new qemu-nbd test (Jeff, 01-02/20)
                Fix memory leak (Stefan, 06/20)
                Use local_err for bdrv_file_open (Kevin, 09-11-12/20)

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                |  11 ++--
 block/curl.c               |  13 ++---
 block/gluster.c            |  28 ++++-----
 block/iscsi.c              | 142 +++++++++++++++++++++++----------------------
 block/nbd.c                |  43 +++++++-------
 block/parallels.c          |   3 +-
 block/qcow.c               |  15 ++---
 block/qcow2.c              |   2 +-
 block/qed.c                |  16 ++---
 block/vdi.c                |  29 +++++----
 block/vhdx.c               |  21 +++----
 block/vmdk.c               | 123 +++++++++++++++++++++++++--------------
 block/vpc.c                |   3 +-
 block/vvfat.c              |   9 +--
 include/block/nbd.h        |   6 --
 nbd.c                      |  66 ---------------------
 qemu-nbd.c                 |  52 +++++++++++++++++
 tests/qemu-iotests/051.out |   4 +-
 tests/qemu-iotests/059.out |   6 +-
 20 files changed, 307 insertions(+), 288 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v3 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 ++++++-------
 tests/qemu-iotests/051.out |  2 ++
 2 files changed, 8 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);
 
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
-- 
1.8.5.3

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On 02/17/2014 06:43 AM, 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
> 

> +    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.");

Pre-existing, but we tend to not use trailing '.' in error messages...

> -            return -EINVAL;
> +        } else {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> +                          "must be specified.");

so you might as well fix the old one and not add a new instance of it.

But that's minor enough that I don't mind:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v3 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 ------------
 tests/qemu-iotests/051.out |  6 ++----
 4 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index fd89083..2378802 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];
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
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 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 v3 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 v3 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 c97c040..95a1030 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 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>
---
 block/iscsi.c | 103 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 95a1030..05dd50d 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;
@@ -1120,27 +1114,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;
     }
@@ -1151,13 +1143,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;
     }
@@ -1166,21 +1158,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;
     }
@@ -1188,10 +1181,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;
@@ -1203,14 +1201,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;
     }
@@ -1218,7 +1216,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);
@@ -1236,14 +1236,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;
         }
@@ -1256,14 +1257,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;
         }
@@ -1354,14 +1355,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 v3 06/20] iscsi: correctly propagate errors in iscsi_open

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 02:43:53PM +0100, Paolo Bonzini wrote:
> 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 | 103 ++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v3 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 v3 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 v3 09/20] cow: correctly propagate errors

[Paolo Bonzini]

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

diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..0564744 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;
@@ -346,16 +346,14 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
 
     ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
     ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
                          &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 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..7edb3cc 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 v3 11/20] qcow: correctly propagate errors

[Paolo Bonzini]

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

diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..a915bc3 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;
     }
@@ -686,16 +688,14 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
 
     ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
     ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
                          &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
-- 
1.8.5.3

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

[Paolo Bonzini]

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

diff --git a/block/qed.c b/block/qed.c
index b9ca7ac..b13ef8a 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,
@@ -566,16 +567,14 @@ static int qed_create(const char *filename, uint32_t cluster_size,
 
     ret = bdrv_create_file(filename, NULL, &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
     ret = bdrv_file_open(&bs, filename, NULL, NULL,
                          BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return ret;
     }
 
@@ -665,7 +664,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 v3 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 55689cf..bd3081b 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

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

[Jeff Cody]

On Mon, Feb 17, 2014 at 02:44:00PM +0100, Paolo Bonzini wrote:
> 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 55689cf..bd3081b 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
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH v3 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 v3 15/20] vmdk: extract vmdk_read_desc

[Paolo Bonzini]

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

diff --git a/block/vmdk.c b/block/vmdk.c
index ff6f5ee..c834512 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 +848,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 v3 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 | 55 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index c834512..0ebb732 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)
@@ -576,7 +576,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;
         }
     }
 
@@ -727,16 +733,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);
@@ -820,8 +822,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;
             }
@@ -844,20 +852,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;
@@ -875,20 +876,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;
         }
@@ -907,10 +913,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 v3 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 0ebb732..d3858b0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -885,20 +885,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 v3 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 d3858b0..ba2b6f5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -572,6 +572,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);
@@ -641,8 +642,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
@@ -654,7 +655,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;
     }
 
@@ -670,8 +671,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 v3 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..4d6403f 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 0564744..9603347 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..3f588f5 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 a915bc3..b273b2f 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 0b4335c..4f7a3d1 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 b13ef8a..236d985 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..f3c6acf 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 ba2b6f5..54ecbd6 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -748,7 +748,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;
     }
 }
@@ -861,7 +862,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..82bf248 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 v3 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 f3c6acf..1966d62 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 v3 00/20] Improve bdrv_open error messages

[Fam Zheng]

On Mon, 02/17 14:43, 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,
> and as a consequence it also avoids abuse of errno numbers.
> 
> 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
> 
>         v2->v3:
>                 Rebase for new qemu-nbd test (Jeff, 01-02/20)
>                 Fix memory leak (Stefan, 06/20)
>                 Use local_err for bdrv_file_open (Kevin, 09-11-12/20)
> 

Did an incremental review from v1 where I already left reviewed-by's for most
patches, and this revision looks good to me. The only minor comment is on v19
about error message text (I replied in v2 series), but it needn't stop merging.
Thanks, the series:

Reviewed-by: Fam Zheng <famz@redhat.com>

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

[Kevin Wolf]

Am 17.02.2014 um 14:43 hat Paolo Bonzini geschrieben:
> 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,
> and as a consequence it also avoids abuse of errno numbers.
> 
> 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.

Thanks, applied to the block branch.

Kevin

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

[Paolo Bonzini]

Il 19/02/2014 15:06, Kevin Wolf ha scritto:
> Thanks, applied to the block branch.

Conflicts with master... to help you rebasing I put my own version at 
block-open-errors on git://github.com/bonzini/qemu.git.

Paolo

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

[Kevin Wolf]

Am 20.02.2014 um 18:29 hat Paolo Bonzini geschrieben:
> Il 19/02/2014 15:06, Kevin Wolf ha scritto:
> >Thanks, applied to the block branch.
> 
> Conflicts with master... to help you rebasing I put my own version
> at block-open-errors on git://github.com/bonzini/qemu.git.

Thanks, but rebasing is easier than removing patches, pulling and
revalidating that there were no other changes. I hope my rebase didn't
break anything - if it does, complain to Markus.

Kevin

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

[Paolo Bonzini]

Il 21/02/2014 12:08, Kevin Wolf ha scritto:
> Am 20.02.2014 um 18:29 hat Paolo Bonzini geschrieben:
>> Il 19/02/2014 15:06, Kevin Wolf ha scritto:
>>> Thanks, applied to the block branch.
>>
>> Conflicts with master... to help you rebasing I put my own version
>> at block-open-errors on git://github.com/bonzini/qemu.git.
> 
> Thanks, but rebasing is easier than removing patches, pulling and
> revalidating that there were no other changes. I hope my rebase didn't
> break anything - if it does, complain to Markus.

All fine.  The idea was more to let you do a double check with

   diff \
      <(git log -p --reverse kwolf/block~20..kwolf/block) \
      <(git log -p --reverse bonzini/block-open-errors~20..bonzini/block-open-errors)

Paolo

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

[Kevin Wolf]

Am 21.02.2014 um 12:37 hat Paolo Bonzini geschrieben:
> Il 21/02/2014 12:08, Kevin Wolf ha scritto:
> > Am 20.02.2014 um 18:29 hat Paolo Bonzini geschrieben:
> >> Il 19/02/2014 15:06, Kevin Wolf ha scritto:
> >>> Thanks, applied to the block branch.
> >>
> >> Conflicts with master... to help you rebasing I put my own version
> >> at block-open-errors on git://github.com/bonzini/qemu.git.
> > 
> > Thanks, but rebasing is easier than removing patches, pulling and
> > revalidating that there were no other changes. I hope my rebase didn't
> > break anything - if it does, complain to Markus.
> 
> All fine.  The idea was more to let you do a double check with
> 
>    diff \
>       <(git log -p --reverse kwolf/block~20..kwolf/block) \
>       <(git log -p --reverse bonzini/block-open-errors~20..bonzini/block-open-errors)

Ah, okay, right, I could have done that instead of only comparing
to the old version. Though it's spelt 'git-backport-diff -u
bonzini/block-open-errors' ;-)

Kevin