[Qemu-devel] [PATCH 0/3] qemu-img: error path fixes

[Qemu-devel] [PATCH 0/3] qemu-img: error path fixes

[Stefan Hajnoczi]

I noticed a few minor issues when looking at cache flag parsing.  This series
cleans them up.

Stefan Hajnoczi (3):
  qemu-img: fix img_commit() error return value
  qemu-img: fix img_compare() flags error path
  qemu-img: always goto out in img_snapshot() error paths

 qemu-img.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[Stefan Hajnoczi]

The img_commit() return value is a process exit code.  Use 1 for failure
instead of -1.  The other failure paths in this function already use 1.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index c843420..dc3adb5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -771,7 +771,7 @@ static int img_commit(int argc, char **argv)
     ret = bdrv_parse_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
-        return -1;
+        return 1;
     }
 
     bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[Max Reitz]

On 26.08.2014 20:17, Stefan Hajnoczi wrote:
> The img_commit() return value is a process exit code.  Use 1 for failure
> instead of -1.  The other failure paths in this function already use 1.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   qemu-img.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On 08/26/2014 12:17 PM, Stefan Hajnoczi wrote:
> The img_commit() return value is a process exit code.  Use 1 for failure
> instead of -1.  The other failure paths in this function already use 1.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index c843420..dc3adb5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -771,7 +771,7 @@ static int img_commit(int argc, char **argv)
>      ret = bdrv_parse_cache_flags(cache, &flags);
>      if (ret < 0) {
>          error_report("Invalid cache option: %s", cache);
> -        return -1;
> +        return 1;

Nothing against this patch (you're consistent with the surrounding code,
and most of qemu for that matter), but it highlights why I'm a fan of
'return EXIT_FAILURE' instead of 'return 1' in functions that return an
exit status, because that makes it a lot more obvious _why_ I'm
returning a non-negative number to represent failure.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[John Snow]

On 08/26/2014 03:31 PM, Eric Blake wrote:
> On 08/26/2014 12:17 PM, Stefan Hajnoczi wrote:
>> The img_commit() return value is a process exit code.  Use 1 for failure
>> instead of -1.  The other failure paths in this function already use 1.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   qemu-img.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index c843420..dc3adb5 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -771,7 +771,7 @@ static int img_commit(int argc, char **argv)
>>       ret = bdrv_parse_cache_flags(cache, &flags);
>>       if (ret < 0) {
>>           error_report("Invalid cache option: %s", cache);
>> -        return -1;
>> +        return 1;
>
> Nothing against this patch (you're consistent with the surrounding code,
> and most of qemu for that matter), but it highlights why I'm a fan of
> 'return EXIT_FAILURE' instead of 'return 1' in functions that return an
> exit status, because that makes it a lot more obvious _why_ I'm
> returning a non-negative number to represent failure.
>

"Hey, good point."

jsnow@local ~/s/qemu> git grep 'return 1;' | wc -l
842

"oh."

This patch is probably fine -- is there some larger scheme we want to 
cook up within the style guide for advocating things like return code 
and error reporting standards?

It seems to me like the preferred style for errors and returns has 
changed several times and there's not a good concrete rule (written) at 
the moment. It might be worth touching the CODING_GUIDE and/or HACKING 
files with recommendations, if we can agree on some consistent set of 
rules, like getting rid of error_set(...) and not using positive, 
un-named integers to represent errors.

-- 
—js

Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 08/26/2014 03:31 PM, Eric Blake wrote:
>> On 08/26/2014 12:17 PM, Stefan Hajnoczi wrote:
>>> The img_commit() return value is a process exit code.  Use 1 for failure
>>> instead of -1.  The other failure paths in this function already use 1.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>   qemu-img.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index c843420..dc3adb5 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -771,7 +771,7 @@ static int img_commit(int argc, char **argv)
>>>       ret = bdrv_parse_cache_flags(cache, &flags);
>>>       if (ret < 0) {
>>>           error_report("Invalid cache option: %s", cache);
>>> -        return -1;
>>> +        return 1;
>>
>> Nothing against this patch (you're consistent with the surrounding code,
>> and most of qemu for that matter), but it highlights why I'm a fan of
>> 'return EXIT_FAILURE' instead of 'return 1' in functions that return an
>> exit status, because that makes it a lot more obvious _why_ I'm
>> returning a non-negative number to represent failure.

For me, EXIT_FAILURE carries a smell.

In main(), the fact that we're returning an exit code is trivial.

Elsewhere, if all you want to return is "succeeded / failed" information
encoded as integer, stick to the usual 0/-1 convention, and leave
mapping that to exit codes to callers.

If you need to return one of several error codes, EXIT_FAILURE is of no
use.

> "Hey, good point."
>
> jsnow@local ~/s/qemu> git grep 'return 1;' | wc -l
> 842
>
> "oh."
>
> This patch is probably fine -- is there some larger scheme we want to
> cook up within the style guide for advocating things like return code
> and error reporting standards?
>
> It seems to me like the preferred style for errors and returns has
> changed several times and there's not a good concrete rule (written)
> at the moment. It might be worth touching the CODING_GUIDE and/or
> HACKING files with recommendations, if we can agree on some consistent
> set of rules, like getting rid of error_set(...) and not using
> positive, un-named integers to represent errors.

Yes, we could use some written guidance on returning errors.  I haven't
found the time to write.  When I can spare a bit of time for errors, I
tend to invest it in killing obsolete error reporting interfaces, so I
don't have to write guidance on them.

[Qemu-devel] [PATCH 2/3] qemu-img: fix img_compare() flags error path

[Stefan Hajnoczi]

If img_compare() fails to parse the cache flags the goto out3 code path
will call qemu_progress_end().  Make sure we actually call
qemu_progress_init() first.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index dc3adb5..01750b7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1018,6 +1018,9 @@ static int img_compare(int argc, char **argv)
     filename1 = argv[optind++];
     filename2 = argv[optind++];
 
+    /* Initialize before goto out */
+    qemu_progress_init(progress, 2.0);
+
     flags = BDRV_O_FLAGS;
     ret = bdrv_parse_cache_flags(cache, &flags);
     if (ret < 0) {
@@ -1026,9 +1029,6 @@ static int img_compare(int argc, char **argv)
         goto out3;
     }
 
-    /* Initialize before goto out */
-    qemu_progress_init(progress, 2.0);
-
     bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
     if (!bs1) {
         error_report("Can't open file %s", filename1);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] qemu-img: fix img_compare() flags error path

[Max Reitz]

On 26.08.2014 20:17, Stefan Hajnoczi wrote:
> If img_compare() fails to parse the cache flags the goto out3 code path
> will call qemu_progress_end().  Make sure we actually call
> qemu_progress_init() first.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   qemu-img.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH 3/3] qemu-img: always goto out in img_snapshot() error paths

[Stefan Hajnoczi]

The out label has the qemu_progress_end() and other cleanup calls.
Always goto out in error paths so the cleanup happens.

Note that bdrv_unref(NULL) is safe.  We just need to initialize bs to
NULL at the top of the function.

We can now remove the obsolete bs_old_backing = NULL and bs_new_backing
= NULL for safe mode.  Originally it was necessary in commit 3e85c6fd
("qemu-img rebase") but became useless in commit c2abcce ("qemu-img:
avoid calling exit(1) to release resources properly") because the
variables are already initialized during declaration.

Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 01750b7..ac2e21b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2323,7 +2323,7 @@ static int img_snapshot(int argc, char **argv)
 
 static int img_rebase(int argc, char **argv)
 {
-    BlockDriverState *bs, *bs_old_backing = NULL, *bs_new_backing = NULL;
+    BlockDriverState *bs = NULL, *bs_old_backing = NULL, *bs_new_backing = NULL;
     BlockDriver *old_backing_drv, *new_backing_drv;
     char *filename;
     const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
@@ -2395,14 +2395,14 @@ static int img_rebase(int argc, char **argv)
     ret = bdrv_parse_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
-        return -1;
+        goto out;
     }
 
     src_flags = BDRV_O_FLAGS;
     ret = bdrv_parse_cache_flags(src_cache, &src_flags);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
-        return -1;
+        goto out;
     }
 
     /*
@@ -2413,7 +2413,8 @@ static int img_rebase(int argc, char **argv)
      */
     bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
     if (!bs) {
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     /* Find the right drivers for the backing files */
@@ -2439,11 +2440,7 @@ static int img_rebase(int argc, char **argv)
     }
 
     /* For safe rebasing we need to compare old and new backing file */
-    if (unsafe) {
-        /* Make the compiler happy */
-        bs_old_backing = NULL;
-        bs_new_backing = NULL;
-    } else {
+    if (!unsafe) {
         char backing_name[1024];
 
         bs_old_backing = bdrv_new("old_backing", &error_abort);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 3/3] qemu-img: always goto out in img_snapshot() error paths

[Max Reitz]

On 26.08.2014 20:17, Stefan Hajnoczi wrote:
> The out label has the qemu_progress_end() and other cleanup calls.
> Always goto out in error paths so the cleanup happens.
>
> Note that bdrv_unref(NULL) is safe.  We just need to initialize bs to
> NULL at the top of the function.
>
> We can now remove the obsolete bs_old_backing = NULL and bs_new_backing
> = NULL for safe mode.  Originally it was necessary in commit 3e85c6fd
> ("qemu-img rebase") but became useless in commit c2abcce ("qemu-img:
> avoid calling exit(1) to release resources properly") because the
> variables are already initialized during declaration.
>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   qemu-img.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] qemu-img: always goto out in img_snapshot() error paths

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The out label has the qemu_progress_end() and other cleanup calls.
> Always goto out in error paths so the cleanup happens.
>
> Note that bdrv_unref(NULL) is safe.  We just need to initialize bs to
> NULL at the top of the function.
>
> We can now remove the obsolete bs_old_backing = NULL and bs_new_backing
> = NULL for safe mode.  Originally it was necessary in commit 3e85c6fd
> ("qemu-img rebase") but became useless in commit c2abcce ("qemu-img:
> avoid calling exit(1) to release resources properly") because the
> variables are already initialized during declaration.

Please mention here that you fix exit code.from -1 to 1.

>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
[...]

Re: [Qemu-devel] [PATCH 3/3] qemu-img: always goto out in img_snapshot() error paths

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Wed, Aug 27, 2014 at 08:35:33AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > The out label has the qemu_progress_end() and other cleanup calls.
> > Always goto out in error paths so the cleanup happens.
> >
> > Note that bdrv_unref(NULL) is safe.  We just need to initialize bs to
> > NULL at the top of the function.
> >
> > We can now remove the obsolete bs_old_backing = NULL and bs_new_backing
> > = NULL for safe mode.  Originally it was necessary in commit 3e85c6fd
> > ("qemu-img rebase") but became useless in commit c2abcce ("qemu-img:
> > avoid calling exit(1) to release resources properly") because the
> > variables are already initialized during declaration.
> 
> Please mention here that you fix exit code.from -1 to 1.

Fixed up when merging

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] qemu-img: error path fixes

[John Snow]

On 08/26/2014 02:17 PM, Stefan Hajnoczi wrote:
> I noticed a few minor issues when looking at cache flag parsing.  This series
> cleans them up.
>
> Stefan Hajnoczi (3):
>    qemu-img: fix img_commit() error return value
>    qemu-img: fix img_compare() flags error path
>    qemu-img: always goto out in img_snapshot() error paths
>
>   qemu-img.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
>

Reviewed-by: John Snow <jsnow@redhat.com>

-- 
—js

Re: [Qemu-devel] [PATCH 0/3] qemu-img: error path fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Tue, Aug 26, 2014 at 07:17:53PM +0100, Stefan Hajnoczi wrote:
> I noticed a few minor issues when looking at cache flag parsing.  This series
> cleans them up.
> 
> Stefan Hajnoczi (3):
>   qemu-img: fix img_commit() error return value
>   qemu-img: fix img_compare() flags error path
>   qemu-img: always goto out in img_snapshot() error paths
> 
>  qemu-img.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]