[Qemu-devel] [RFC 0/7] block: bdrv_img_create(): propagate errors

[Qemu-devel] [RFC 0/7] block: bdrv_img_create(): propagate errors

[Luiz Capitulino]

I'm calling this an RFC because I did it on hurry and it's almost untested,
but I wanted to drop it for early review while I'm out for a public holiday :)

This should improve qmp_transaction() error messages on bdrv_img_create()
failure quite a bit. Also, the "formatting" message is not printed to stdout
anymore when in QMP.

Luiz Capitulino (6):
  block: bdrv_img_create(): add param_ret argument
  block: bdrv_img_create(): move param printing to qemu-img
  block: bdrv_img_create(): add Error ** argument
  qemu-img: img_create(): use Error object
  qmp: qmp_transaction(): pass Error object to bdrv_img_create()
  block: bdrv_img_create(): drop unused code

Paolo Bonzini (1):
  error: add error_set_errno and error_setg_errno

 block.c    | 69 +++++++++++++++++++++++++++-----------------------------------
 block.h    |  7 ++++---
 blockdev.c | 13 ++++++------
 error.c    | 28 +++++++++++++++++++++++++
 error.h    |  9 ++++++++
 qemu-img.c | 18 +++++++++++++---
 6 files changed, 93 insertions(+), 51 deletions(-)

-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 1/7] error: add error_set_errno and error_setg_errno

[Luiz Capitulino]

From: Paolo Bonzini <pbonzini@redhat.com>

These functions help maintaining homogeneous formatting of error
messages that include strerror values.

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 error.c | 28 ++++++++++++++++++++++++++++
 error.h |  9 +++++++++
 2 files changed, 37 insertions(+)

diff --git a/error.c b/error.c
index 1f05fc4..128d88c 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     *errp = err;
 }
 
+void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
+                     const char *fmt, ...)
+{
+    Error *err;
+    char *msg1;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+    assert(*errp == NULL);
+
+    err = g_malloc0(sizeof(*err));
+
+    va_start(ap, fmt);
+    msg1 = g_strdup_vprintf(fmt, ap);
+    if (os_errno != 0) {
+        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
+        g_free(msg1);
+    } else {
+        err->msg = msg1;
+    }
+    va_end(ap);
+    err->err_class = err_class;
+
+    *errp = err;
+}
+
 Error *error_copy(const Error *err)
 {
     Error *err_new;
diff --git a/error.h b/error.h
index da7fed3..4d52e73 100644
--- a/error.h
+++ b/error.h
@@ -30,10 +30,19 @@ typedef struct Error Error;
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
 /**
+ * Set an indirect pointer to an error given a ErrorClass value and a
+ * printf-style human message, followed by a strerror() string if
+ * @os_error is not zero.
+ */
+void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+
+/**
  * Same as error_set(), but sets a generic error
  */
 #define error_setg(err, fmt, ...) \
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_errno(err, os_error, fmt, ...) \
+    error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 
 /**
  * Returns true if an indirect pointer to an error is pointing to a valid
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 2/7] block: bdrv_img_create(): add param_ret argument

[Luiz Capitulino]

If set returns a copy of the parameter list used by the block driver
to create the image.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    | 7 ++++++-
 block.h    | 3 ++-
 blockdev.c | 2 +-
 qemu-img.c | 2 +-
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index e95f613..13cf04d 100644
--- a/block.c
+++ b/block.c
@@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
 
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags)
+                    char *options, uint64_t img_size, int flags,
+                    QEMUOptionParameter **param_ret)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
     }
 
 out:
+    if (param_ret) {
+        *param_ret = append_option_parameters(NULL, param);
+    }
+
     free_option_parameters(create_options);
     free_option_parameters(param);
 
diff --git a/block.h b/block.h
index e2d89d7..6403b12 100644
--- a/block.h
+++ b/block.h
@@ -341,7 +341,8 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags);
+                    char *options, uint64_t img_size, int flags,
+                    QEMUOptionParameter **param_ret);
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
diff --git a/blockdev.c b/blockdev.c
index 99828ad..9dddefd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -783,7 +783,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
             ret = bdrv_img_create(new_image_file, format,
                                   states->old_bs->filename,
                                   states->old_bs->drv->format_name,
-                                  NULL, -1, flags);
+                                  NULL, -1, flags, NULL);
             if (ret) {
                 error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
                 goto delete_and_fail;
diff --git a/qemu-img.c b/qemu-img.c
index f17f187..b841012 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -362,7 +362,7 @@ static int img_create(int argc, char **argv)
     }
 
     ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                          options, img_size, BDRV_O_FLAGS);
+                          options, img_size, BDRV_O_FLAGS, NULL);
 out:
     if (ret) {
         return 1;
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 3/7] block: bdrv_img_create(): move param printing to qemu-img

[Luiz Capitulino]

bdrv_img_create() is being used by the transaction QMP command and
therefore shouldn't print directly to the user.

Move the param printing to qemu-img instead. Has the side effect of
only printing it when the bdrv_img_create() call succeeds, otherwise
we can print errors before the action being taken, eg:

   ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
   qemu-img: /foo/foo: error while creating qcow2: No such file or directory
   Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    |  4 ----
 qemu-img.c | 10 +++++++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 13cf04d..235423e 100644
--- a/block.c
+++ b/block.c
@@ -4411,10 +4411,6 @@ int bdrv_img_create(const char *filename, const char *fmt,
         }
     }
 
-    printf("Formatting '%s', fmt=%s ", filename, fmt);
-    print_option_parameters(param);
-    puts("");
-
     ret = bdrv_create(drv, filename, param);
 
     if (ret < 0) {
diff --git a/qemu-img.c b/qemu-img.c
index b841012..ac66459 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -301,6 +301,7 @@ static int img_create(int argc, char **argv)
     const char *filename;
     const char *base_filename = NULL;
     char *options = NULL;
+    QEMUOptionParameter *params = NULL;
 
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:");
@@ -362,7 +363,14 @@ static int img_create(int argc, char **argv)
     }
 
     ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                          options, img_size, BDRV_O_FLAGS, NULL);
+                          options, img_size, BDRV_O_FLAGS, &params);
+    if (ret == 0 && params) {
+        printf("Formatting '%s', fmt=%s ", filename, fmt);
+        print_option_parameters(params);
+        free_option_parameters(params);
+        puts("");
+    }
+
 out:
     if (ret) {
         return 1;
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 4/7] block: bdrv_img_create(): add Error ** argument

[Luiz Capitulino]

This commit adds an Error ** argument to bdrv_img_create() and set it
appropriately on error.

Callers of bdrv_img_create() pass NULL for the new argument and still
rely on bdrv_img_create()'s return value. Next commits will change
callers to use the Error object instead.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    | 23 +++++++++++++++++++++--
 block.h    |  2 +-
 blockdev.c |  2 +-
 qemu-img.c |  2 +-
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 235423e..3f4bec0 100644
--- a/block.c
+++ b/block.c
@@ -4295,7 +4295,7 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
                     char *options, uint64_t img_size, int flags,
-                    QEMUOptionParameter **param_ret)
+                    QEMUOptionParameter **param_ret, Error **errp)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -4308,6 +4308,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
     drv = bdrv_find_format(fmt);
     if (!drv) {
         error_report("Unknown file format '%s'", fmt);
+        error_setg(errp, "Unknown file format '%s'", fmt);
         ret = -EINVAL;
         goto out;
     }
@@ -4315,6 +4316,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
     proto_drv = bdrv_find_protocol(filename);
     if (!proto_drv) {
         error_report("Unknown protocol '%s'", filename);
+        error_setg(errp, "Unknown protocol '%s'", filename);
         ret = -EINVAL;
         goto out;
     }
@@ -4334,6 +4336,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
         param = parse_option_parameters(options, create_options, param);
         if (param == NULL) {
             error_report("Invalid options for file format '%s'.", fmt);
+            error_setg(errp, "Invalid options for file format '%s'.", fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4344,6 +4347,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
                                  base_filename)) {
             error_report("Backing file not supported for file format '%s'",
                          fmt);
+            error_setg(errp, "Backing file not supported for file format '%s'",
+                       fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4353,6 +4358,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
         if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
             error_report("Backing file format not supported for file "
                          "format '%s'", fmt);
+            error_setg(errp, "Backing file format not supported for file "
+                             "format '%s'", fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4363,6 +4370,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
         if (!strcmp(filename, backing_file->value.s)) {
             error_report("Error: Trying to create an image with the "
                          "same filename as the backing file");
+            error_setg(errp, "Error: Trying to create an image with the "
+                             "same filename as the backing file");
             ret = -EINVAL;
             goto out;
         }
@@ -4374,6 +4383,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
         if (!backing_drv) {
             error_report("Unknown backing file format '%s'",
                          backing_fmt->value.s);
+            error_setg(errp, "Unknown backing file format '%s'",
+                       backing_fmt->value.s);
             ret = -EINVAL;
             goto out;
         }
@@ -4396,7 +4407,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
 
             ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
             if (ret < 0) {
-                error_report("Could not open '%s'", backing_file->value.s);
+                error_setg_errno(errp, -ret, "Could not open '%s'",
+                                 backing_file->value.s);
                 goto out;
             }
             bdrv_get_geometry(bs, &size);
@@ -4406,6 +4418,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
             set_option_parameter(param, BLOCK_OPT_SIZE, buf);
         } else {
             error_report("Image creation needs a size parameter");
+            error_setg(errp, "Image creation needs a size parameter");
             ret = -EINVAL;
             goto out;
         }
@@ -4417,12 +4430,18 @@ int bdrv_img_create(const char *filename, const char *fmt,
         if (ret == -ENOTSUP) {
             error_report("Formatting or formatting option not supported for "
                          "file format '%s'", fmt);
+            error_setg(errp,"Formatting or formatting option not supported for "
+                            "file format '%s'", fmt);
         } else if (ret == -EFBIG) {
             error_report("The image size is too large for file format '%s'",
                          fmt);
+            error_setg(errp, "The image size is too large for file format '%s'",
+                       fmt);
         } else {
             error_report("%s: error while creating %s: %s", filename, fmt,
                          strerror(-ret));
+            error_setg(errp, "%s: error while creating %s: %s", filename, fmt,
+                       strerror(-ret));
         }
     }
 
diff --git a/block.h b/block.h
index 6403b12..70ea52e 100644
--- a/block.h
+++ b/block.h
@@ -342,7 +342,7 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
                     char *options, uint64_t img_size, int flags,
-                    QEMUOptionParameter **param_ret);
+                    QEMUOptionParameter **param_ret, Error **errp);
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
diff --git a/blockdev.c b/blockdev.c
index 9dddefd..01be90f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -783,7 +783,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
             ret = bdrv_img_create(new_image_file, format,
                                   states->old_bs->filename,
                                   states->old_bs->drv->format_name,
-                                  NULL, -1, flags, NULL);
+                                  NULL, -1, flags, NULL, NULL);
             if (ret) {
                 error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
                 goto delete_and_fail;
diff --git a/qemu-img.c b/qemu-img.c
index ac66459..99b8ad1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -363,7 +363,7 @@ static int img_create(int argc, char **argv)
     }
 
     ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                          options, img_size, BDRV_O_FLAGS, &params);
+                          options, img_size, BDRV_O_FLAGS, &params, NULL);
     if (ret == 0 && params) {
         printf("Formatting '%s', fmt=%s ", filename, fmt);
         print_option_parameters(params);
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 5/7] qemu-img: img_create(): use Error object

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-img.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 99b8ad1..18885c6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -302,6 +302,7 @@ static int img_create(int argc, char **argv)
     const char *base_filename = NULL;
     char *options = NULL;
     QEMUOptionParameter *params = NULL;
+    Error *local_err = NULL;
 
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:");
@@ -362,9 +363,12 @@ static int img_create(int argc, char **argv)
         goto out;
     }
 
-    ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                          options, img_size, BDRV_O_FLAGS, &params, NULL);
-    if (ret == 0 && params) {
+    bdrv_img_create(filename, fmt, base_filename, base_fmt,
+                    options, img_size, BDRV_O_FLAGS, &params, &local_err);
+    if (error_is_set(&local_err)) {
+        fprintf(stderr, "qemu-img create: %s\n", error_get_pretty(local_err));
+        error_free(local_err);
+    } else if (params) {
         printf("Formatting '%s', fmt=%s ", filename, fmt);
         print_option_parameters(params);
         free_option_parameters(params);
@@ -372,7 +376,7 @@ static int img_create(int argc, char **argv)
     }
 
 out:
-    if (ret) {
+    if (ret || error_is_set(&local_err)) {
         return 1;
     }
     return 0;
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 6/7] qmp: qmp_transaction(): pass Error object to bdrv_img_create()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 01be90f..af02480 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -701,6 +701,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     int ret = 0;
     BlockdevActionList *dev_entry = dev_list;
     BlkTransactionStates *states, *next;
+    Error *local_err = NULL;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -780,12 +781,12 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
 
         /* create new image w/backing file */
         if (mode != NEW_IMAGE_MODE_EXISTING) {
-            ret = bdrv_img_create(new_image_file, format,
-                                  states->old_bs->filename,
-                                  states->old_bs->drv->format_name,
-                                  NULL, -1, flags, NULL, NULL);
-            if (ret) {
-                error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+            bdrv_img_create(new_image_file, format,
+                            states->old_bs->filename,
+                            states->old_bs->drv->format_name,
+                            NULL, -1, flags, NULL, &local_err);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
                 goto delete_and_fail;
             }
         }
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [RFC 7/7] block: bdrv_img_create(): drop unused code

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c | 41 ++++++-----------------------------------
 block.h |  8 ++++----
 2 files changed, 10 insertions(+), 39 deletions(-)

diff --git a/block.c b/block.c
index 3f4bec0..79e33a0 100644
--- a/block.c
+++ b/block.c
@@ -4292,10 +4292,10 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
     bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
-int bdrv_img_create(const char *filename, const char *fmt,
-                    const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags,
-                    QEMUOptionParameter **param_ret, Error **errp)
+void bdrv_img_create(const char *filename, const char *fmt,
+                     const char *base_filename, const char *base_fmt,
+                     char *options, uint64_t img_size, int flags,
+                     QEMUOptionParameter **param_ret, Error **errp)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -4307,18 +4307,14 @@ int bdrv_img_create(const char *filename, const char *fmt,
     /* Find driver and parse its options */
     drv = bdrv_find_format(fmt);
     if (!drv) {
-        error_report("Unknown file format '%s'", fmt);
         error_setg(errp, "Unknown file format '%s'", fmt);
-        ret = -EINVAL;
-        goto out;
+        return;
     }
 
     proto_drv = bdrv_find_protocol(filename);
     if (!proto_drv) {
-        error_report("Unknown protocol '%s'", filename);
         error_setg(errp, "Unknown protocol '%s'", filename);
-        ret = -EINVAL;
-        goto out;
+        return;
     }
 
     create_options = append_option_parameters(create_options,
@@ -4335,9 +4331,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
     if (options) {
         param = parse_option_parameters(options, create_options, param);
         if (param == NULL) {
-            error_report("Invalid options for file format '%s'.", fmt);
             error_setg(errp, "Invalid options for file format '%s'.", fmt);
-            ret = -EINVAL;
             goto out;
         }
     }
@@ -4345,22 +4339,16 @@ int bdrv_img_create(const char *filename, const char *fmt,
     if (base_filename) {
         if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
                                  base_filename)) {
-            error_report("Backing file not supported for file format '%s'",
-                         fmt);
             error_setg(errp, "Backing file not supported for file format '%s'",
                        fmt);
-            ret = -EINVAL;
             goto out;
         }
     }
 
     if (base_fmt) {
         if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
-            error_report("Backing file format not supported for file "
-                         "format '%s'", fmt);
             error_setg(errp, "Backing file format not supported for file "
                              "format '%s'", fmt);
-            ret = -EINVAL;
             goto out;
         }
     }
@@ -4368,11 +4356,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
     backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
     if (backing_file && backing_file->value.s) {
         if (!strcmp(filename, backing_file->value.s)) {
-            error_report("Error: Trying to create an image with the "
-                         "same filename as the backing file");
             error_setg(errp, "Error: Trying to create an image with the "
                              "same filename as the backing file");
-            ret = -EINVAL;
             goto out;
         }
     }
@@ -4381,11 +4366,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
     if (backing_fmt && backing_fmt->value.s) {
         backing_drv = bdrv_find_format(backing_fmt->value.s);
         if (!backing_drv) {
-            error_report("Unknown backing file format '%s'",
-                         backing_fmt->value.s);
             error_setg(errp, "Unknown backing file format '%s'",
                        backing_fmt->value.s);
-            ret = -EINVAL;
             goto out;
         }
     }
@@ -4417,29 +4399,20 @@ int bdrv_img_create(const char *filename, const char *fmt,
             snprintf(buf, sizeof(buf), "%" PRId64, size);
             set_option_parameter(param, BLOCK_OPT_SIZE, buf);
         } else {
-            error_report("Image creation needs a size parameter");
             error_setg(errp, "Image creation needs a size parameter");
-            ret = -EINVAL;
             goto out;
         }
     }
 
     ret = bdrv_create(drv, filename, param);
-
     if (ret < 0) {
         if (ret == -ENOTSUP) {
-            error_report("Formatting or formatting option not supported for "
-                         "file format '%s'", fmt);
             error_setg(errp,"Formatting or formatting option not supported for "
                             "file format '%s'", fmt);
         } else if (ret == -EFBIG) {
-            error_report("The image size is too large for file format '%s'",
-                         fmt);
             error_setg(errp, "The image size is too large for file format '%s'",
                        fmt);
         } else {
-            error_report("%s: error while creating %s: %s", filename, fmt,
-                         strerror(-ret));
             error_setg(errp, "%s: error while creating %s: %s", filename, fmt,
                        strerror(-ret));
         }
@@ -4456,6 +4429,4 @@ out:
     if (bs) {
         bdrv_delete(bs);
     }
-
-    return ret;
 }
diff --git a/block.h b/block.h
index 70ea52e..957368e 100644
--- a/block.h
+++ b/block.h
@@ -339,10 +339,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
                       int64_t pos, int size);
 
-int bdrv_img_create(const char *filename, const char *fmt,
-                    const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags,
-                    QEMUOptionParameter **param_ret, Error **errp);
+void bdrv_img_create(const char *filename, const char *fmt,
+                     const char *base_filename, const char *base_fmt,
+                     char *options, uint64_t img_size, int flags,
+                     QEMUOptionParameter **param_ret, Error **errp);
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [RFC 3/7] block: bdrv_img_create(): move param printing to qemu-img

[Paolo Bonzini]

Il 11/10/2012 23:27, Luiz Capitulino ha scritto:
> bdrv_img_create() is being used by the transaction QMP command and
> therefore shouldn't print directly to the user.
> 
> Move the param printing to qemu-img instead. Has the side effect of
> only printing it when the bdrv_img_create() call succeeds, otherwise
> we can print errors before the action being taken, eg:
> 
>    ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
>    qemu-img: /foo/foo: error while creating qcow2: No such file or directory
>    Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off

It is a small regression with -monitor stdio (and also with QMP it
doesn't appear anymore in the logs).  Do we care?  What alternatives
exist besides writing a QAPI key-value store and converting the output
QEMUOptionParameters to it (which I'm not suggesting to do)?

Paolo

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c    |  4 ----
>  qemu-img.c | 10 +++++++++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 13cf04d..235423e 100644
> --- a/block.c
> +++ b/block.c
> @@ -4411,10 +4411,6 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          }
>      }
>  
> -    printf("Formatting '%s', fmt=%s ", filename, fmt);
> -    print_option_parameters(param);
> -    puts("");
> -
>      ret = bdrv_create(drv, filename, param);
>  
>      if (ret < 0) {
> diff --git a/qemu-img.c b/qemu-img.c
> index b841012..ac66459 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -301,6 +301,7 @@ static int img_create(int argc, char **argv)
>      const char *filename;
>      const char *base_filename = NULL;
>      char *options = NULL;
> +    QEMUOptionParameter *params = NULL;
>  
>      for(;;) {
>          c = getopt(argc, argv, "F:b:f:he6o:");
> @@ -362,7 +363,14 @@ static int img_create(int argc, char **argv)
>      }
>  
>      ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
> -                          options, img_size, BDRV_O_FLAGS, NULL);
> +                          options, img_size, BDRV_O_FLAGS, &params);
> +    if (ret == 0 && params) {
> +        printf("Formatting '%s', fmt=%s ", filename, fmt);
> +        print_option_parameters(params);
> +        free_option_parameters(params);
> +        puts("");
> +    }
> +
>  out:
>      if (ret) {
>          return 1;
> 

Re: [Qemu-devel] [RFC 0/7] block: bdrv_img_create(): propagate errors

[Paolo Bonzini]

Il 11/10/2012 23:26, Luiz Capitulino ha scritto:
> I'm calling this an RFC because I did it on hurry and it's almost untested,
> but I wanted to drop it for early review while I'm out for a public holiday :)
> 
> This should improve qmp_transaction() error messages on bdrv_img_create()
> failure quite a bit. Also, the "formatting" message is not printed to stdout
> anymore when in QMP.
> 
> Luiz Capitulino (6):
>   block: bdrv_img_create(): add param_ret argument
>   block: bdrv_img_create(): move param printing to qemu-img
>   block: bdrv_img_create(): add Error ** argument
>   qemu-img: img_create(): use Error object
>   qmp: qmp_transaction(): pass Error object to bdrv_img_create()
>   block: bdrv_img_create(): drop unused code
> 
> Paolo Bonzini (1):
>   error: add error_set_errno and error_setg_errno
> 
>  block.c    | 69 +++++++++++++++++++++++++++-----------------------------------
>  block.h    |  7 ++++---
>  blockdev.c | 13 ++++++------
>  error.c    | 28 +++++++++++++++++++++++++
>  error.h    |  9 ++++++++
>  qemu-img.c | 18 +++++++++++++---
>  6 files changed, 93 insertions(+), 51 deletions(-)
> 

Looks good.  We could debate endlessly how to order the patches, but the
idea is fine.

Paolo

Re: [Qemu-devel] [RFC 3/7] block: bdrv_img_create(): move param printing to qemu-img

[Luiz Capitulino]

On Fri, 12 Oct 2012 10:29:37 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 11/10/2012 23:27, Luiz Capitulino ha scritto:
> > bdrv_img_create() is being used by the transaction QMP command and
> > therefore shouldn't print directly to the user.
> > 
> > Move the param printing to qemu-img instead. Has the side effect of
> > only printing it when the bdrv_img_create() call succeeds, otherwise
> > we can print errors before the action being taken, eg:
> > 
> >    ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
> >    qemu-img: /foo/foo: error while creating qcow2: No such file or directory
> >    Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off
> 
> It is a small regression with -monitor stdio (and also with QMP it
> doesn't appear anymore in the logs).  Do we care? 

I don't think so. But if we do, than the current code is also wrong
as it should work with any -monitor device and not only stdio.

IMO, what's there today was really meant to be displayed when running
qemu-img.

> What alternatives
> exist besides writing a QAPI key-value store and converting the output
> QEMUOptionParameters to it (which I'm not suggesting to do)?

Yes, the right way to have this would be to add it as a return value
of the qmp command calling bdrv_img_create() (certainly not doable now
for the transaction command due to compatibility issues).

And/or add a query-block-image command and/or extend query-block to
display the image options...