| Message ID | f00f8aeaa6e7f11d66bddc3593f6af0b45915150.1366817130.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
δΊ 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>
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> >
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)
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(-)