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

[Thomas Huth]

On 19.05.2017 07:54, Markus Armbruster wrote:
> Thomas Huth <address@hidden> writes:
> 
>> On 18.05.2017 15:35, Paolo Bonzini wrote:
>>>
>>>
>>> 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...
>>
>> Hmm, maybe we should simply remove "-usbdevice serial" right now already
>> ... ? The vendorid/productid parameter handling has been broken since
>> QEMU v0.14 already and nobody ever complained, so I guess hardly anybody
>> is using "-usbdevice serial" anymore ... so I tend to simply remove it
>> directly instead of going through the typical "mark-as-deprecated ->
>> wait-two-release-cycles -> finally-remove-it" process here...
>>
>> Paolo, Gerd, what do you think?
> 
> Being broken counts as being deprecated, I'd say.
> 
> But was -usbdevice serial broken?  Or just its two optional (and
> somewhat exotic) parameters?

Just the two exotic parameters. But I agree with Gerd: If you google for
the option to see how people are using the usbdevice option, you almost
only find pages that describe "-usbdevice mouse/keyboard/tablet" or host
passthrough. Sometimes also "storage". But hardly anything about
"serial". However, I've now also found a bug where someone complains
about the vendorid/productid problem:

 https://bugs.launchpad.net/qemu/+bug/1180924

So that's an indication that at least some few people used "-usbdevice
serial" in the past ... so let's fix that coverity warning now instead,
then add some proper deprecation warnings to the "-usbdevice" parameter
and remove all that legacy stuff next year...

 Thomas