[Qemu-devel] [PATCH 0/8] block: bdrv_img_create(): propagate errors

[Qemu-devel] [PATCH 0/8] block: bdrv_img_create(): propagate errors

[Luiz Capitulino]

This series is a first step to propagate errors from bdrv_open(), but it also
improves qmp_transaction() error messages on bdrv_img_create() failures plus
some other improvements.

Please, check individual patches for details.

o From the RFC

 - Better testing
 - improved changelogs
 - small simplifications

Luiz Capitulino (7):
  block: bdrv_img_create(): add param_list argument
  block: bdrv_img_create(): move parameter list printing to qemu-img
  block: bdrv_img_create(): add Error ** argument
  qemu-img: img_create(): use Error object
  qemu-img: img_create(): simplify
  qmp: qmp_transaction(): pass Error object to bdrv_img_create()
  block: bdrv_img_create(): drop unused error handling 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 | 26 +++++++++++++++--------
 6 files changed, 95 insertions(+), 57 deletions(-)

-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 1/8] 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.

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] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Luiz Capitulino]

If set returns a copy of the parameter list used by the block driver
to create the new 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..254a5c2 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_list)
 {
     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_list && ret == 0) {
+        *param_list = append_option_parameters(NULL, param);
+    }
+
     free_option_parameters(create_options);
     free_option_parameters(param);
 
diff --git a/block.h b/block.h
index e2d89d7..55d13e6 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_list);
 
 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

Re: [Qemu-devel] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Kevin Wolf]

Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> If set returns a copy of the parameter list used by the block driver
> to create the new 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..254a5c2 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_list)
>  {
>      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_list && ret == 0) {
> +        *param_list = append_option_parameters(NULL, param);
> +    }

If you put this above the out: label, the ret == 0 check wouldn't be
necessary.

Kevin

Re: [Qemu-devel] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Luiz Capitulino]

On Thu, 18 Oct 2012 13:57:45 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > If set returns a copy of the parameter list used by the block driver
> > to create the new 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..254a5c2 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_list)
> >  {
> >      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_list && ret == 0) {
> > +        *param_list = append_option_parameters(NULL, param);
> > +    }
> 
> If you put this above the out: label, the ret == 0 check wouldn't be
> necessary.

I'll maintain it it there but will drop the ret check, so that we're
able to return the options on error too and thus keep the "Formatting"
message the way it's today.

Re: [Qemu-devel] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Luiz Capitulino]

On Thu, 18 Oct 2012 10:33:30 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 18 Oct 2012 13:57:45 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > > If set returns a copy of the parameter list used by the block driver
> > > to create the new 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..254a5c2 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_list)
> > >  {
> > >      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_list && ret == 0) {
> > > +        *param_list = append_option_parameters(NULL, param);
> > > +    }
> > 
> > If you put this above the out: label, the ret == 0 check wouldn't be
> > necessary.
> 
> I'll maintain it it there but will drop the ret check, so that we're
> able to return the options on error too and thus keep the "Formatting"
> message the way it's today.

Actually my idea didn't work. I can think of the following options:

 1. Change it to "Formatted" as you suggested. Cons: will only be
    printed after the image is created

 2. Print "Formatting 'foo', 'qcow2' " before the image is created
    and the options afterwards. Cons: weird?

 3. Don't change this. Cons: will keep spitting stuff to stdout while in
    QMP

I think we should do 2.

Re: [Qemu-devel] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Kevin Wolf]

Am 18.10.2012 19:18, schrieb Luiz Capitulino:
> On Thu, 18 Oct 2012 10:33:30 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Thu, 18 Oct 2012 13:57:45 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>>>> If set returns a copy of the parameter list used by the block driver
>>>> to create the new 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..254a5c2 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_list)
>>>>  {
>>>>      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_list && ret == 0) {
>>>> +        *param_list = append_option_parameters(NULL, param);
>>>> +    }
>>>
>>> If you put this above the out: label, the ret == 0 check wouldn't be
>>> necessary.
>>
>> I'll maintain it it there but will drop the ret check, so that we're
>> able to return the options on error too and thus keep the "Formatting"
>> message the way it's today.
> 
> Actually my idea didn't work. I can think of the following options:
> 
>  1. Change it to "Formatted" as you suggested. Cons: will only be
>     printed after the image is created
> 
>  2. Print "Formatting 'foo', 'qcow2' " before the image is created
>     and the options afterwards. Cons: weird?
> 
>  3. Don't change this. Cons: will keep spitting stuff to stdout while in
>     QMP
> 
> I think we should do 2.

How about leaving the code printing the message where it is, but putting
an if (!cur_mon) around it?

Kevin

Re: [Qemu-devel] [PATCH 2/8] block: bdrv_img_create(): add param_list argument

[Luiz Capitulino]

On Fri, 19 Oct 2012 11:13:39 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 18.10.2012 19:18, schrieb Luiz Capitulino:
> > On Thu, 18 Oct 2012 10:33:30 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >> On Thu, 18 Oct 2012 13:57:45 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> >>>> If set returns a copy of the parameter list used by the block driver
> >>>> to create the new 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..254a5c2 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_list)
> >>>>  {
> >>>>      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_list && ret == 0) {
> >>>> +        *param_list = append_option_parameters(NULL, param);
> >>>> +    }
> >>>
> >>> If you put this above the out: label, the ret == 0 check wouldn't be
> >>> necessary.
> >>
> >> I'll maintain it it there but will drop the ret check, so that we're
> >> able to return the options on error too and thus keep the "Formatting"
> >> message the way it's today.
> > 
> > Actually my idea didn't work. I can think of the following options:
> > 
> >  1. Change it to "Formatted" as you suggested. Cons: will only be
> >     printed after the image is created
> > 
> >  2. Print "Formatting 'foo', 'qcow2' " before the image is created
> >     and the options afterwards. Cons: weird?
> > 
> >  3. Don't change this. Cons: will keep spitting stuff to stdout while in
> >     QMP
> > 
> > I think we should do 2.
> 
> How about leaving the code printing the message where it is, but putting
> an if (!cur_mon) around it?

So ugly that I prefer to drop this patch from my series and revisit this
later :)

[Qemu-devel] [PATCH 3/8] block: bdrv_img_create(): move parameter list printing to qemu-img

[Luiz Capitulino]

Today, bdrv_img_create() prints the parameter list used to create the
new image to stdout, like this:

   Formatting '/tmp/a', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off

As the transaction QMP command calls bdrv_img_create(), this message
is also printed when using QMP.

This commit moves the printing of the parameter list to qemu-img instead.
This way we avoid printing it in QMP and from whatever bdrv_img_create()
usage we might have in the future.

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

diff --git a/block.c b/block.c
index 254a5c2..bdb53af 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..e482443 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,17 @@ 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) {
+        goto out;
+    }
+
+    assert(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

Re: [Qemu-devel] [PATCH 3/8] block: bdrv_img_create(): move parameter list printing to qemu-img

[Kevin Wolf]

Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> Today, bdrv_img_create() prints the parameter list used to create the
> new image to stdout, like this:
> 
>    Formatting '/tmp/a', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off
> 
> As the transaction QMP command calls bdrv_img_create(), this message
> is also printed when using QMP.
> 
> This commit moves the printing of the parameter list to qemu-img instead.
> This way we avoid printing it in QMP and from whatever bdrv_img_create()
> usage we might have in the future.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

I believe the idea was that this message is printed before actually
creating the image, which in the case of preallocation could take a while.

> ---
>  block.c    |  4 ----
>  qemu-img.c | 13 ++++++++++++-
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 254a5c2..bdb53af 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..e482443 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,17 @@ 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) {
> +        goto out;
> +    }
> +
> +    assert(params);
> +    printf("Formatting '%s', fmt=%s ", filename, fmt);

If we do want to move the message to the end of the operation,
s/Formatting/Formatted/ would make more sense. Could possibly break some
scripts, though.

> +    print_option_parameters(params);
> +    free_option_parameters(params);
> +    puts("");
> +
>  out:
>      if (ret) {
>          return 1;

Kevin

Re: [Qemu-devel] [PATCH 3/8] block: bdrv_img_create(): move parameter list printing to qemu-img

[Kevin Wolf]

Am 18.10.2012 14:01, schrieb Kevin Wolf:
> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>> Today, bdrv_img_create() prints the parameter list used to create the
>> new image to stdout, like this:
>>
>>    Formatting '/tmp/a', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off
>>
>> As the transaction QMP command calls bdrv_img_create(), this message
>> is also printed when using QMP.
>>
>> This commit moves the printing of the parameter list to qemu-img instead.
>> This way we avoid printing it in QMP and from whatever bdrv_img_create()
>> usage we might have in the future.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> I believe the idea was that this message is printed before actually
> creating the image, which in the case of preallocation could take a while.

The other thing is that I think you don't print the message any more
when creating the image fails. Did you check that qemu-iotests doesn't
need any updates?

Kevin

Re: [Qemu-devel] [PATCH 3/8] block: bdrv_img_create(): move parameter list printing to qemu-img

[Luiz Capitulino]

On Thu, 18 Oct 2012 14:16:43 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 18.10.2012 14:01, schrieb Kevin Wolf:
> > Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> >> Today, bdrv_img_create() prints the parameter list used to create the
> >> new image to stdout, like this:
> >>
> >>    Formatting '/tmp/a', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off
> >>
> >> As the transaction QMP command calls bdrv_img_create(), this message
> >> is also printed when using QMP.
> >>
> >> This commit moves the printing of the parameter list to qemu-img instead.
> >> This way we avoid printing it in QMP and from whatever bdrv_img_create()
> >> usage we might have in the future.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > 
> > I believe the idea was that this message is printed before actually
> > creating the image, which in the case of preallocation could take a while.
> 
> The other thing is that I think you don't print the message any more
> when creating the image fails. Did you check that qemu-iotests doesn't
> need any updates?

I didn't. But I'll preserve today's behavior for v2 as I mentioned in my
last email.

[Qemu-devel] [PATCH 4/8] 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 bdb53af..83bcab5 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_list)
+                    QEMUOptionParameter **param_list, 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 55d13e6..49d89a1 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_list);
+                    QEMUOptionParameter **param_list, 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 e482443..12fb6c2 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) {
         goto out;
     }
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 4/8] block: bdrv_img_create(): add Error ** argument

[Kevin Wolf]

Am 17.10.2012 21:35, schrieb 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 bdb53af..83bcab5 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_list)
> +                    QEMUOptionParameter **param_list, 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);

This is the only error_report() that is dropped in this patch. Not sure
what is the best patch to drop them, but consistency wouldn't hurt.

If we want to keep the series bisectable, i.e. qemu-img output stays the
same for each patch, you'd have to drop them in 5/8, I think.

Kevin

Re: [Qemu-devel] [PATCH 4/8] block: bdrv_img_create(): add Error ** argument

[Luiz Capitulino]

On Thu, 18 Oct 2012 14:08:05 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.10.2012 21:35, schrieb 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 bdb53af..83bcab5 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_list)
> > +                    QEMUOptionParameter **param_list, 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);
> 
> This is the only error_report() that is dropped in this patch. Not sure
> what is the best patch to drop them, but consistency wouldn't hurt.

That's correct. I dropped it by accident because I was working on this
series and the bdrv_open() conversion at the same time and probably
mixed the two.

Will fix it for v2.

> 
> If we want to keep the series bisectable, i.e. qemu-img output stays the
> same for each patch, you'd have to drop them in 5/8, I think.
> 
> Kevin
> 

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

[Luiz Capitulino]

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

diff --git a/qemu-img.c b/qemu-img.c
index 12fb6c2..dfde588 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) {
+    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: %s\n", error_get_pretty(local_err));
+        error_free(local_err);
+        ret = -1;
         goto out;
     }
 
-- 
1.7.12.315.g682ce8b

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Kevin Wolf]

Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-img.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 12fb6c2..dfde588 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) {
> +    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: %s\n", error_get_pretty(local_err));

This should use error_report() instead of adding the "qemu-img:" manually.

Kevin

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Luiz Capitulino]

On Thu, 18 Oct 2012 14:11:40 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-img.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 12fb6c2..dfde588 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) {
> > +    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: %s\n", error_get_pretty(local_err));
> 
> This should use error_report() instead of adding the "qemu-img:" manually.

If you want to do it for consistency with the current code then ok,
I'll do it for v2. But I disagree qemu-img should keep using error_report(),
printing to hmp for example is something beyond the interests of a tool like
qemu-img.

I think we should use something like this for the tools:

static void print_error(const Error **errp)
{
    fprintf(stderr, "%s: %s\n", progname, error_get_pretty(errp));
}

Where progname is set during initialization.

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Kevin Wolf]

Am 18.10.2012 15:49, schrieb Luiz Capitulino:
> On Thu, 18 Oct 2012 14:11:40 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  qemu-img.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 12fb6c2..dfde588 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) {
>>> +    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: %s\n", error_get_pretty(local_err));
>>
>> This should use error_report() instead of adding the "qemu-img:" manually.
> 
> If you want to do it for consistency with the current code then ok,
> I'll do it for v2. But I disagree qemu-img should keep using error_report(),
> printing to hmp for example is something beyond the interests of a tool like
> qemu-img.

qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you
want to replace it, replace it with a copy of qemu-error.c that only
removes the monitor_vprintf() case. That is, in particular, leave all of
the loc_*() functionality there, because this is something that is meant
for use in command line parsers.

That qemu-img doesn't use these functions yet should be fixed. It has
some good use cases for them.

Kevin

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 18.10.2012 15:49, schrieb Luiz Capitulino:
>> On Thu, 18 Oct 2012 14:11:40 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>> 
>>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>> ---
>>>>  qemu-img.c | 10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index 12fb6c2..dfde588 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) {
>>>> +    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: %s\n", error_get_pretty(local_err));
>>>
>>> This should use error_report() instead of adding the "qemu-img:" manually.
>> 
>> If you want to do it for consistency with the current code then ok,
>> I'll do it for v2. But I disagree qemu-img should keep using error_report(),
>> printing to hmp for example is something beyond the interests of a tool like
>> qemu-img.
>
> qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you
> want to replace it, replace it with a copy of qemu-error.c that only
> removes the monitor_vprintf() case. That is, in particular, leave all of
> the loc_*() functionality there, because this is something that is meant
> for use in command line parsers.

Strongly agreed on use of error_report().  Buys us uniform error
messages that can point to the location causing the error.  The fact
that error_report() does the right thing in monitor context is detail.

I don't see why purging a few monitor references from tools is worth
copying the file.  Stubbing out cur_mon and monitor_vprintf() in tools
is just fine, in my opinion.

> That qemu-img doesn't use these functions yet should be fixed. It has
> some good use cases for them.

Indeed.

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Kevin Wolf]

Am 23.10.2012 11:58, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 18.10.2012 15:49, schrieb Luiz Capitulino:
>>> On Thu, 18 Oct 2012 14:11:40 +0200
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>>> ---
>>>>>  qemu-img.c | 10 +++++++---
>>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>>> index 12fb6c2..dfde588 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) {
>>>>> +    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: %s\n", error_get_pretty(local_err));
>>>>
>>>> This should use error_report() instead of adding the "qemu-img:" manually.
>>>
>>> If you want to do it for consistency with the current code then ok,
>>> I'll do it for v2. But I disagree qemu-img should keep using error_report(),
>>> printing to hmp for example is something beyond the interests of a tool like
>>> qemu-img.
>>
>> qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you
>> want to replace it, replace it with a copy of qemu-error.c that only
>> removes the monitor_vprintf() case. That is, in particular, leave all of
>> the loc_*() functionality there, because this is something that is meant
>> for use in command line parsers.
> 
> Strongly agreed on use of error_report().  Buys us uniform error
> messages that can point to the location causing the error.  The fact
> that error_report() does the right thing in monitor context is detail.
> 
> I don't see why purging a few monitor references from tools is worth
> copying the file.  Stubbing out cur_mon and monitor_vprintf() in tools
> is just fine, in my opinion.

Purging monitor references from the code isn't worth it. Becoming
independent from functions also used by the monitor, which keep being
subject of heated discussions, might be worth it, because I don't feel
like joining these discussions more often than absolutely necessary.

It was just my offer in case Luiz insists on getting rid of
error_report() in the tools, not something I'd advocate myself.

Kevin

Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object

[Luiz Capitulino]

On Tue, 23 Oct 2012 12:08:56 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> >> qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you
> >> want to replace it, replace it with a copy of qemu-error.c that only
> >> removes the monitor_vprintf() case. That is, in particular, leave all of
> >> the loc_*() functionality there, because this is something that is meant
> >> for use in command line parsers.
> > 
> > Strongly agreed on use of error_report().  Buys us uniform error
> > messages that can point to the location causing the error.  The fact
> > that error_report() does the right thing in monitor context is detail.
> > 
> > I don't see why purging a few monitor references from tools is worth
> > copying the file.  Stubbing out cur_mon and monitor_vprintf() in tools
> > is just fine, in my opinion.
> 
> Purging monitor references from the code isn't worth it.

It depends. Basically, any function that does any version of:

 if (qmp) {
   /* do qmp stuff */
 } else if (hmp) {
   /* do hmp stuff */
 } else if (command-line) {
   /* do cmd-line stuff */
 }

Is very likely wrong, as it's mixing three different layers. The problem
this gives ranges from a bloated function to errors not getting correctly
reported or a change in "do hmp stuff" affects qmp or any other layer in
the mix.

Of course that it's completely fine for a function to be called from more than
one layer, but it has to be generic enough so that it doesn't make assumptions
wrt the context it's being used.

Of course that we don't have to fix everything right now, and a few places
can make minimal use of such functions in way that it will never hurt.

But at a minimum we shouldn't add more of these.

> Becoming
> independent from functions also used by the monitor, which keep being
> subject of heated discussions, might be worth it, because I don't feel
> like joining these discussions more often than absolutely necessary.
> 
> It was just my offer in case Luiz insists on getting rid of
> error_report() in the tools, not something I'd advocate myself.
> 
> Kevin
> 

[Qemu-devel] [PATCH 6/8] qemu-img: img_create(): simplify

[Luiz Capitulino]

Drop unneeded goto and ret variable.

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

diff --git a/qemu-img.c b/qemu-img.c
index dfde588..0634402 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -294,7 +294,7 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
 
 static int img_create(int argc, char **argv)
 {
-    int c, ret = 0;
+    int c;
     uint64_t img_size = -1;
     const char *fmt = "raw";
     const char *base_fmt = NULL;
@@ -352,15 +352,13 @@ static int img_create(int argc, char **argv)
             error_report("Invalid image size specified! You may use k, M, G or "
                   "T suffixes for ");
             error_report("kilobytes, megabytes, gigabytes and terabytes.");
-            ret = -1;
-            goto out;
+            return 1;
         }
         img_size = (uint64_t)sval;
     }
 
     if (options && is_help_option(options)) {
-        ret = print_block_option_help(filename, fmt);
-        goto out;
+        return print_block_option_help(filename, fmt);
     }
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
@@ -368,8 +366,7 @@ static int img_create(int argc, char **argv)
     if (error_is_set(&local_err)) {
         fprintf(stderr, "qemu-img: %s\n", error_get_pretty(local_err));
         error_free(local_err);
-        ret = -1;
-        goto out;
+        return 1;
     }
 
     assert(params);
@@ -378,10 +375,6 @@ static int img_create(int argc, char **argv)
     free_option_parameters(params);
     puts("");
 
-out:
-    if (ret) {
-        return 1;
-    }
     return 0;
 }
 
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 7/8] 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] [PATCH 8/8] block: bdrv_img_create(): drop unused error handling 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 83bcab5..5e8184f 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_list, 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_list, 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 49d89a1..f26e52f 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_list, 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_list, Error **errp);
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
-- 
1.7.12.315.g682ce8b