[12/21] qga: Fix latent guest-get-fsinfo error handling bug
diff mbox series

Message ID 20191130194240.10517-13-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
build_guest_fsinfo_for_virtual_device() crashes when
build_guest_fsinfo_for_device() fails and its @errp argument is null.
Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-posix.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 4:29 p.m. UTC | #1
30.11.2019 22:42, Markus Armbruster wrote:
> build_guest_fsinfo_for_virtual_device() crashes when
> build_guest_fsinfo_for_device() fails and its @errp argument is null.
> Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Actually, all such bugs should be fixed by my auto-generated series..
And, if fixing by hand, it may be better to teach this function to return
int, than propagation is not needed.

> ---
>   qga/commands-posix.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165dae..0be527ccb8 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1049,6 +1049,7 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
>                                                     GuestFilesystemInfo *fs,
>                                                     Error **errp)
>   {
> +    Error *err = NULL;
>       DIR *dir;
>       char *dirpath;
>       struct dirent *entry;
> @@ -1078,10 +1079,11 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
>   
>               g_debug(" slave device '%s'", entry->d_name);
>               path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
> -            build_guest_fsinfo_for_device(path, fs, errp);
> +            build_guest_fsinfo_for_device(path, fs, &err);
>               g_free(path);
>   
> -            if (*errp) {
> +            if (err) {
> +                error_propagate(errp, err);
>                   break;
>               }
>           }
>
Markus Armbruster Dec. 6, 2019, 7:58 a.m. UTC | #2
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 30.11.2019 22:42, Markus Armbruster wrote:
>> build_guest_fsinfo_for_virtual_device() crashes when
>> build_guest_fsinfo_for_device() fails and its @errp argument is null.
>> Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".
>> 
>> The bug can't bite as no caller actually passes null.  Fix it anyway.
>> 
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Actually, all such bugs should be fixed by my auto-generated series..

I see.  I didn't consider that.

One advantage of my manual fixing is a clearer record of the flaws in
git.  It should also keep your work simpler, which is always a good idea
for massive, mechanical patches.

> And, if fixing by hand, it may be better to teach this function to return
> int, than propagation is not needed.

I went for the minimal fix.

I believe returning something useful is better.  It also matches the
GError conventions.  Deviating from them in this point was a mistake.

Touching up functions to return something useful by hand is a lot of
work, though: functions have many returns, and we have many functions in
need of this touch-up.  Some clever Coccinellery might be able to pull
it off.  I haven't tried.

However, your clever "auto propagation" Coccinellery makes such a change
less compelling, because it hides the propagation.

Thanks!

Patch
diff mbox series

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165dae..0be527ccb8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1049,6 +1049,7 @@  static void build_guest_fsinfo_for_virtual_device(char const *syspath,
                                                   GuestFilesystemInfo *fs,
                                                   Error **errp)
 {
+    Error *err = NULL;
     DIR *dir;
     char *dirpath;
     struct dirent *entry;
@@ -1078,10 +1079,11 @@  static void build_guest_fsinfo_for_virtual_device(char const *syspath,
 
             g_debug(" slave device '%s'", entry->d_name);
             path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
-            build_guest_fsinfo_for_device(path, fs, errp);
+            build_guest_fsinfo_for_device(path, fs, &err);
             g_free(path);
 
-            if (*errp) {
+            if (err) {
+                error_propagate(errp, err);
                 break;
             }
         }