Patchwork [5/8] qemu-img: img_create(): use Error object

login
register
mail settings
Submitter Luiz Capitulino
Date Oct. 17, 2012, 7:35 p.m.
Message ID <1350502556-4885-6-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/192133/
State New
Headers show

Comments

Luiz Capitulino - Oct. 17, 2012, 7:35 p.m.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-img.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Kevin Wolf - Oct. 18, 2012, 12:11 p.m.
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
Luiz Capitulino - Oct. 18, 2012, 1:49 p.m.
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.
Kevin Wolf - Oct. 18, 2012, 2:02 p.m.
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
Markus Armbruster - Oct. 23, 2012, 9:58 a.m.
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.
Kevin Wolf - Oct. 23, 2012, 10:08 a.m.
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
Luiz Capitulino - Oct. 23, 2012, 1:07 p.m.
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
>

Patch

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;
     }