Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/05/2017 14:21, Gerd Hoffmann wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> When starting QEMU with the legacy USB serial device like this:
> 
>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
> 
> it currently aborts since the vendorid property does not exist
> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
> 
>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>                      '.vendorid' not found
>  Aborted (core dumped)
> 
> Fix this crash by issuing a more friendly error message instead
> (and simplify the code also a little bit this way).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-serial.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 6d5137383b..83a4f0e6fb 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>  {
>      USBDevice *dev;
>      Chardev *cdrv;
> -    uint32_t vendorid = 0, productid = 0;
>      char label[32];
>      static int index;
>  
>      while (*filename && *filename != ':') {
>          const char *p;
> -        char *e;
> +
>          if (strstart(filename, "vendorid=", &p)) {
> -            vendorid = strtol(p, &e, 16);
> -            if (e == p || (*e && *e != ',' && *e != ':')) {
> -                error_report("bogus vendor ID %s", p);
> -                return NULL;
> -            }
> -            filename = e;
> +            error_report("vendorid is not supported anymore");
> +            return NULL;
>          } else if (strstart(filename, "productid=", &p)) {
> -            productid = strtol(p, &e, 16);
> -            if (e == p || (*e && *e != ',' && *e != ':')) {
> -                error_report("bogus product ID %s", p);
> -                return NULL;
> -            }
> -            filename = e;
> +            error_report("productid is not supported anymore");
> +            return NULL;
>          } else {
>              error_report("unrecognized serial USB option %s", filename);
>              return NULL;

All breanches of the "if" now return NULL, so the "while" loop in turn
can become an

    if (*filename && *filename != ':') {
    }

and the "while (*filename == ',')" subloop can go away, replaced by just
"return NULL".

Even better, the "if (!*filename)" if just below can be moved first.

Paolo
> @@ -554,10 +545,7 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>  
>      dev = usb_create(bus, "usb-serial");
>      qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
> -    if (vendorid)
> -        qdev_prop_set_uint16(&dev->qdev, "vendorid", vendorid);
> -    if (productid)
> -        qdev_prop_set_uint16(&dev->qdev, "productid", productid);
> +
>      return dev;
>  }
>  
>