| Message ID | 918a1a8bde813010ce5576613614ad1d58f10942.1366127809.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > Later in the patch series we will use this function few times. s/few/a few/ > This will avoid of duplicating the code. s/of// > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > qemu-img.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > Later in the patch series we will use this function few times. > This will avoid of duplicating the code. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > qemu-img.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 31627b0..dbacdb7 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(const char *msg, Error *err) > +{ > + if (error_is_set(&err)) { > + error_report("%s: %s", msg, 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("qemu-img create failed", local_err); > } This makes a change to the error message that isn't mentioned in the commit message. It should definitely be mentioned there, but I'm not even sure if it's a good change. Today you get something like: $ qemu-img create -f foo test.img qemu-img: Unknown file format 'foo' With the patch applied it becomes: $ qemu-img create -f foo test.img qemu-img: qemu-img create failed: Unknown file format 'foo' Does this add any useful information or does it just make the error message longer? I feel it's the latter. Kevin
On 18.4.2013 13:44, Kevin Wolf wrote: > Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: >> Later in the patch series we will use this function few times. >> This will avoid of duplicating the code. >> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> qemu-img.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 31627b0..dbacdb7 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(const char *msg, Error *err) >> +{ >> + if (error_is_set(&err)) { >> + error_report("%s: %s", msg, 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("qemu-img create failed", local_err); >> } > > This makes a change to the error message that isn't mentioned in the > commit message. It should definitely be mentioned there, but I'm not > even sure if it's a good change. Today you get something like: > > $ qemu-img create -f foo test.img > qemu-img: Unknown file format 'foo' > > With the patch applied it becomes: > > $ qemu-img create -f foo test.img > qemu-img: qemu-img create failed: Unknown file format 'foo' > > Does this add any useful information or does it just make the error > message longer? I feel it's the latter. > > Kevin > Thanks for pointing this out, it should be qemu_img_handle_error("create failed", local_err); and later in patch series also qemu_img_handle_error("snapshot create failed", local_err); Is that OK after this change? Pavel
Am 18.04.2013 um 13:52 hat Pavel Hrdina geschrieben: > On 18.4.2013 13:44, Kevin Wolf wrote: > >Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > >>Later in the patch series we will use this function few times. > >>This will avoid of duplicating the code. > >> > >>Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > >>--- > >> qemu-img.c | 17 +++++++++++------ > >> 1 file changed, 11 insertions(+), 6 deletions(-) > >> > >>diff --git a/qemu-img.c b/qemu-img.c > >>index 31627b0..dbacdb7 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(const char *msg, Error *err) > >>+{ > >>+ if (error_is_set(&err)) { > >>+ error_report("%s: %s", msg, 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("qemu-img create failed", local_err); > >> } > > > >This makes a change to the error message that isn't mentioned in the > >commit message. It should definitely be mentioned there, but I'm not > >even sure if it's a good change. Today you get something like: > > > > $ qemu-img create -f foo test.img > > qemu-img: Unknown file format 'foo' > > > >With the patch applied it becomes: > > > > $ qemu-img create -f foo test.img > > qemu-img: qemu-img create failed: Unknown file format 'foo' > > > >Does this add any useful information or does it just make the error > >message longer? I feel it's the latter. > > > >Kevin > > > > Thanks for pointing this out, it should be > qemu_img_handle_error("create failed", local_err); > > and later in patch series also > qemu_img_handle_error("snapshot create failed", local_err); > > Is that OK after this change? I would in fact leave the error message as it is today. The "create failed" doesn't help me understand the error better. I already know that it's a create command that failed, because it was me who called 'qemu-img create'. Kevin
On 18.4.2013 14:59, Kevin Wolf wrote: > Am 18.04.2013 um 13:52 hat Pavel Hrdina geschrieben: >> On 18.4.2013 13:44, Kevin Wolf wrote: >>> Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: >>>> Later in the patch series we will use this function few times. >>>> This will avoid of duplicating the code. >>>> >>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >>>> --- >>>> qemu-img.c | 17 +++++++++++------ >>>> 1 file changed, 11 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/qemu-img.c b/qemu-img.c >>>> index 31627b0..dbacdb7 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(const char *msg, Error *err) >>>> +{ >>>> + if (error_is_set(&err)) { >>>> + error_report("%s: %s", msg, 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("qemu-img create failed", local_err); >>>> } >>> >>> This makes a change to the error message that isn't mentioned in the >>> commit message. It should definitely be mentioned there, but I'm not >>> even sure if it's a good change. Today you get something like: >>> >>> $ qemu-img create -f foo test.img >>> qemu-img: Unknown file format 'foo' >>> >>> With the patch applied it becomes: >>> >>> $ qemu-img create -f foo test.img >>> qemu-img: qemu-img create failed: Unknown file format 'foo' >>> >>> Does this add any useful information or does it just make the error >>> message longer? I feel it's the latter. >>> >>> Kevin >>> >> >> Thanks for pointing this out, it should be >> qemu_img_handle_error("create failed", local_err); >> >> and later in patch series also >> qemu_img_handle_error("snapshot create failed", local_err); >> >> Is that OK after this change? > > I would in fact leave the error message as it is today. The "create > failed" doesn't help me understand the error better. I already know that > it's a create command that failed, because it was me who called 'qemu-img > create'. > > Kevin > Yes that's true and make sense, but what if another tool internally uses the qemu-img command? I know that the tool could print/return/whatever proper error message, but qemu-img could help with that printing more information. Pavel
On Thu, 18 Apr 2013 15:09:27 +0200 Pavel Hrdina <phrdina@redhat.com> wrote: > On 18.4.2013 14:59, Kevin Wolf wrote: > > Am 18.04.2013 um 13:52 hat Pavel Hrdina geschrieben: > >> On 18.4.2013 13:44, Kevin Wolf wrote: > >>> Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > >>>> Later in the patch series we will use this function few times. > >>>> This will avoid of duplicating the code. > >>>> > >>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > >>>> --- > >>>> qemu-img.c | 17 +++++++++++------ > >>>> 1 file changed, 11 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/qemu-img.c b/qemu-img.c > >>>> index 31627b0..dbacdb7 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(const char *msg, Error *err) > >>>> +{ > >>>> + if (error_is_set(&err)) { > >>>> + error_report("%s: %s", msg, 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("qemu-img create failed", local_err); > >>>> } > >>> > >>> This makes a change to the error message that isn't mentioned in the > >>> commit message. It should definitely be mentioned there, but I'm not > >>> even sure if it's a good change. Today you get something like: > >>> > >>> $ qemu-img create -f foo test.img > >>> qemu-img: Unknown file format 'foo' > >>> > >>> With the patch applied it becomes: > >>> > >>> $ qemu-img create -f foo test.img > >>> qemu-img: qemu-img create failed: Unknown file format 'foo' > >>> > >>> Does this add any useful information or does it just make the error > >>> message longer? I feel it's the latter. > >>> > >>> Kevin > >>> > >> > >> Thanks for pointing this out, it should be > >> qemu_img_handle_error("create failed", local_err); > >> > >> and later in patch series also > >> qemu_img_handle_error("snapshot create failed", local_err); > >> > >> Is that OK after this change? > > > > I would in fact leave the error message as it is today. The "create > > failed" doesn't help me understand the error better. I already know that > > it's a create command that failed, because it was me who called 'qemu-img > > create'. > > > > Kevin > > > > Yes that's true and make sense, but what if another tool internally uses > the qemu-img command? I know that the tool could print/return/whatever > proper error message, but qemu-img could help with that printing more > information. Then the tool should prepend any relevant information. We can't guess all use cases.
diff --git a/qemu-img.c b/qemu-img.c index 31627b0..dbacdb7 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(const char *msg, Error *err) +{ + if (error_is_set(&err)) { + error_report("%s: %s", msg, 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("qemu-img create failed", local_err); } static void dump_json_image_check(ImageCheck *check, bool quiet)
Later in the patch series we will use this function few times. This will avoid of duplicating the code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- qemu-img.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)