| Message ID | 20191130194240.10517-13-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
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; > } > } >
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!
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; } }
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(-)