| Message ID | 20201021071520.2168877-4-armbru@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qga: Fix several guest-get-devices issues | expand |
On Wed, Oct 21, 2020 at 11:19 AM Markus Armbruster <armbru@redhat.com> wrote: > The Error ** argument must be NULL, &error_abort, &error_fatal, or a > pointer to a variable containing NULL. Passing an argument of the > latter kind twice without clearing it in between is wrong: if the > first call sets an error, it no longer points to NULL for the second > call. > > qmp_guest_get_devices() is wrong that way: it calls error_setg() in a > loop. > > If no iteration fails, the function returns a value and sets no error. > Okay. > > If exactly one iteration fails, the function returns a value and sets > an error. Wrong. > > If multiple iterations fail, the function trips error_setv()'s > assertion. > > Fix it to return immediately on error. > > Perhaps the failure to convert the driver version to UTF-8 should not > be an error. We could simply not report the botched version string > instead. > > Drop a superfluous continue while there. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qga/commands-win32.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index b01616a992..1efe3ba076 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > **errp) > device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL); > if (device->driver_name == NULL) { > error_setg(errp, "conversion to utf8 failed (driver name)"); > - continue; > + return NULL; > } > slog("querying device: %s", device->driver_name); > hw_ids = ga_get_hardware_ids(dev_info_data.DevInst); > @@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > **errp) > NULL, NULL); > if (device->driver_version == NULL) { > error_setg(errp, "conversion to utf8 failed (driver > version)"); > - continue; > + return NULL; > } > device->has_driver_version = true; > > @@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error > **errp) > cur_item->next = item; > cur_item = item; > } > - continue; > } > > if (dev_info != INVALID_HANDLE_VALUE) { > -- > 2.26.2 > > > -- Marc-André Lureau <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 21, 2020 at 11:19 AM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The Error ** argument must be NULL, &error_abort, &error_fatal, or a<br> pointer to a variable containing NULL. Passing an argument of the<br> latter kind twice without clearing it in between is wrong: if the<br> first call sets an error, it no longer points to NULL for the second<br> call.<br> <br> qmp_guest_get_devices() is wrong that way: it calls error_setg() in a<br> loop.<br> <br> If no iteration fails, the function returns a value and sets no error.<br> Okay.<br> <br> If exactly one iteration fails, the function returns a value and sets<br> an error. Wrong.<br> <br> If multiple iterations fail, the function trips error_setv()'s<br> assertion.<br> <br> Fix it to return immediately on error.<br> <br> Perhaps the failure to convert the driver version to UTF-8 should not<br> be an error. We could simply not report the botched version string<br> instead.<br> <br> Drop a superfluous continue while there.<br> <br> Signed-off-by: Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>><br></blockquote><div><br></div><div><div>Reviewed-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com">marcandre.lureau@redhat.com</a>></div><div> </div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> ---<br> qga/commands-win32.c | 5 ++---<br> 1 file changed, 2 insertions(+), 3 deletions(-)<br> <br> diff --git a/qga/commands-win32.c b/qga/commands-win32.c<br> index b01616a992..1efe3ba076 100644<br> --- a/qga/commands-win32.c<br> +++ b/qga/commands-win32.c<br> @@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)<br> device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL);<br> if (device->driver_name == NULL) {<br> error_setg(errp, "conversion to utf8 failed (driver name)");<br> - continue;<br> + return NULL;<br> }<br> slog("querying device: %s", device->driver_name);<br> hw_ids = ga_get_hardware_ids(dev_info_data.DevInst);<br> @@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)<br> NULL, NULL);<br> if (device->driver_version == NULL) {<br> error_setg(errp, "conversion to utf8 failed (driver version)");<br> - continue;<br> + return NULL;<br> }<br> device->has_driver_version = true;<br> <br> @@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)<br> cur_item->next = item;<br> cur_item = item;<br> }<br> - continue;<br> }<br> <br> if (dev_info != INVALID_HANDLE_VALUE) {<br> -- <br> 2.26.2<br> <br> <br> </blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 10/21/20 9:15 AM, Markus Armbruster wrote: > The Error ** argument must be NULL, &error_abort, &error_fatal, or a > pointer to a variable containing NULL. Passing an argument of the > latter kind twice without clearing it in between is wrong: if the > first call sets an error, it no longer points to NULL for the second > call. > > qmp_guest_get_devices() is wrong that way: it calls error_setg() in a > loop. > > If no iteration fails, the function returns a value and sets no error. > Okay. > > If exactly one iteration fails, the function returns a value and sets > an error. Wrong. > > If multiple iterations fail, the function trips error_setv()'s > assertion. > > Fix it to return immediately on error. > > Perhaps the failure to convert the driver version to UTF-8 should not > be an error. We could simply not report the botched version string > instead. > > Drop a superfluous continue while there. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > qga/commands-win32.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index b01616a992..1efe3ba076 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL); if (device->driver_name == NULL) { error_setg(errp, "conversion to utf8 failed (driver name)"); - continue; + return NULL; } slog("querying device: %s", device->driver_name); hw_ids = ga_get_hardware_ids(dev_info_data.DevInst); @@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) NULL, NULL); if (device->driver_version == NULL) { error_setg(errp, "conversion to utf8 failed (driver version)"); - continue; + return NULL; } device->has_driver_version = true; @@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp) cur_item->next = item; cur_item = item; } - continue; } if (dev_info != INVALID_HANDLE_VALUE) {
The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. qmp_guest_get_devices() is wrong that way: it calls error_setg() in a loop. If no iteration fails, the function returns a value and sets no error. Okay. If exactly one iteration fails, the function returns a value and sets an error. Wrong. If multiple iterations fail, the function trips error_setv()'s assertion. Fix it to return immediately on error. Perhaps the failure to convert the driver version to UTF-8 should not be an error. We could simply not report the botched version string instead. Drop a superfluous continue while there. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qga/commands-win32.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)