RE: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Markus Armbruster <armbru@xxxxxxxxxx>
> Sent: 24 April 2020 20:20
> To: qemu-devel@xxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Anthony Perard 
> <anthony.perard@xxxxxxxxxx>; Paul
> Durrant <paul@xxxxxxx>; Gerd Hoffmann <kraxel@xxxxxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host 
> errors
> 
> usbback_portid_add() leaks the error when qdev_device_add() fails.
> Fix that.  While there, use the error to improve the error message.
> 
> The qemu_opts_from_qdict() similarly leaks on failure.  But any
> failure there is a programming error.  Pass &error_abort.
> 
> Fixes: 816ac92ef769f9ffc534e49a1bb6177bddce7aa2
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: Paul Durrant <paul@xxxxxxx>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
> ---
>  hw/usb/xen-usb.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> index 961190d0f7..42643c3390 100644
> --- a/hw/usb/xen-usb.c
> +++ b/hw/usb/xen-usb.c
> @@ -30,6 +30,7 @@
>  #include "hw/usb.h"
>  #include "hw/xen/xen-legacy-backend.h"
>  #include "monitor/qdev.h"
> +#include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> 
> @@ -755,13 +756,15 @@ static void usbback_portid_add(struct usbback_info 
> *usbif, unsigned port,
>      qdict_put_int(qdict, "port", port);
>      qdict_put_int(qdict, "hostbus", atoi(busid));
>      qdict_put_str(qdict, "hostport", portname);
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> -    if (local_err) {
> -        goto err;
> -    }
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
> +                                &error_abort);
>      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, 
> &local_err));
>      if (!usbif->ports[port - 1].dev) {
> -        goto err;
> +        qobject_unref(qdict);
> +        xen_pv_printf(&usbif->xendev, 0,
> +                      "device %s could not be opened: %s\n",
> +                      busid, error_get_pretty(local_err));
> +        error_free(local_err);

Previously the goto caused the function to bail out. Should there not be a 
'return' here?

>      }
>      qobject_unref(qdict);
>      speed = usbif->ports[port - 1].dev->speed;
> @@ -793,11 +796,6 @@ static void usbback_portid_add(struct usbback_info 
> *usbif, unsigned port,
>      usbback_hotplug_enq(usbif, port);
> 
>      TR_BUS(&usbif->xendev, "port %d attached\n", port);
> -    return;
> -
> -err:
> -    qobject_unref(qdict);
> -    xen_pv_printf(&usbif->xendev, 0, "device %s could not be opened\n", 
> busid);
>  }
> 
>  static void usbback_process_port(struct usbback_info *usbif, unsigned port)
> --
> 2.21.1