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

[Paolo Bonzini]

On 18/05/2017 15:22, Thomas Huth wrote:
> On 18.05.2017 14:00, Paolo Bonzini wrote:
>>
>>
>> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>>> From: Thomas Huth <address@hidden>
>>>
>>> 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 <address@hidden>
>>> Message-id: address@hidden
>>> Signed-off-by: Gerd Hoffmann <address@hidden>
>>> ---
>>>  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.
> 
> Feel free to send an additional cleanup patch ... otherwise, I'd say let
> it bitrot for another year and we then remove it completely together
> with all the other "-usbdevice" functions...

Well, Coverity reports it so I'd rather keep it clean...

Paolo