Patchwork [v2,01/12] qemu-img: introduce qemu_img_handle_error()

login
register
mail settings
Submitter Pavel Hrdina
Date April 24, 2013, 3:31 p.m.
Message ID <f00f8aeaa6e7f11d66bddc3593f6af0b45915150.1366817130.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/239231/
State New
Headers show

Comments

Pavel Hrdina - April 24, 2013, 3:31 p.m.
Later in the patch series we will use this function a few times.
This will avoid duplicating the code.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 qemu-img.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
Eric Blake - April 24, 2013, 4:44 p.m.
On 04/24/2013 09:31 AM, Pavel Hrdina wrote:
> Later in the patch series we will use this function a few times.
> This will avoid duplicating the code.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  qemu-img.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)


>  
> +static int qemu_img_handle_error(Error *err)
> +{
> +    if (error_is_set(&err)) {
> +        error_report("%s", error_get_pretty(err));
> +        error_free(err);
> +        return 1;
> +    }
> +    return 0;

Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
instead of 0/1 is a bit nicer at expressing why we chose a positive
value; but that would be a separate cleanup to all of qemu-img.c.
Hence, I have no problems giving:

Reviewed-by: Eric Blake <eblake@redhat.com>
Wayne Xia - April 25, 2013, 2:53 a.m.
于 2013-4-25 0:44, Eric Blake 写道:
> On 04/24/2013 09:31 AM, Pavel Hrdina wrote:
>> Later in the patch series we will use this function a few times.
>> This will avoid duplicating the code.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   qemu-img.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>
>
>>
>> +static int qemu_img_handle_error(Error *err)
>> +{
>> +    if (error_is_set(&err)) {
>> +        error_report("%s", error_get_pretty(err));
>> +        error_free(err);
>> +        return 1;
>> +    }
>> +    return 0;
>
> Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
> instead of 0/1 is a bit nicer at expressing why we chose a positive
> value; but that would be a separate cleanup to all of qemu-img.c.
> Hence, I have no problems giving:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
   Maybe an incode comments like:
+/* Returns 1 on error. */

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Eric Blake - April 25, 2013, 3 a.m.
On 04/24/2013 08:53 PM, Wenchao Xia wrote:

>>>
>>> +static int qemu_img_handle_error(Error *err)
>>> +{
>>> +    if (error_is_set(&err)) {
>>> +        error_report("%s", error_get_pretty(err));
>>> +        error_free(err);
>>> +        return 1;
>>> +    }
>>> +    return 0;
>>
>> Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
>> instead of 0/1 is a bit nicer at expressing why we chose a positive
>> value; but that would be a separate cleanup to all of qemu-img.c.
>> Hence, I have no problems giving:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>   Maybe an incode comments like:
> +/* Returns 1 on error. */

That would also help.  My main concern was that +1 on error is unusual
compared to most of qemu that returns <0 on error.  The _reason_ it is a
positive number is because we are really returning EXIT_FAILURE (a
well-defined constant from system headers) - but calling the number by
its name is smarter than just using a magic number without explanation.
 But as I already said, that's a bigger problem for a different series.

> 
> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>

Patch

diff --git a/qemu-img.c b/qemu-img.c
index cd096a1..ab83fbe 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -322,6 +322,16 @@  static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
     return 0;
 }
 
+static int qemu_img_handle_error(Error *err)
+{
+    if (error_is_set(&err)) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+        return 1;
+    }
+    return 0;
+}
+
 static int img_create(int argc, char **argv)
 {
     int c;
@@ -400,13 +410,8 @@  static int img_create(int argc, char **argv)
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
                     options, img_size, BDRV_O_FLAGS, &local_err, quiet);
-    if (error_is_set(&local_err)) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
-        return 1;
-    }
 
-    return 0;
+    return qemu_img_handle_error(local_err);
 }
 
 static void dump_json_image_check(ImageCheck *check, bool quiet)