[PULL,6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties
diff mbox

Message ID 20170512122158.32032-7-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann May 12, 2017, 12:21 p.m. UTC
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(-)

Comments

Paolo Bonzini May 18, 2017, noon UTC | #1
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;
>  }
>  
>
Thomas Huth May 18, 2017, 1:22 p.m. UTC | #2
On 18.05.2017 14:00, Paolo Bonzini wrote:
> 
> 
> 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.

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...

 Thomas
Paolo Bonzini May 18, 2017, 1:35 p.m. UTC | #3
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 <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.
> 
> 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
Thomas Huth May 18, 2017, 2:05 p.m. UTC | #4
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 <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.
>>
>> 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?

 Thomas
Gerd Hoffmann May 18, 2017, 4:12 p.m. UTC | #5
Hi,

> >> 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...

It will be clean once removed, I likewise wouldn't put too much effort
into code which is scheduled for removal anyway, especially for more or
less cosmetic issues like unreachable code.

> 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...

Hmm, there is little reason to set vendorid + productid, so that alone
is no strong indication that -usbdevice serial is unused.  But
nevertheless I think it usb-serial is one of the rarely used devices, so
we could try ...

Order is probably along these lines:

usb hid devices are used alot (tablet on x86, all on !x86).
usb-storage comes next I guess.
usb-host (aka pass-through) is important too of course.

Everything else (serial, net, bluetooth) is largely unused I guess.

ccid, uas and mtp are new enough to not carry around -usbdevice
compatibility support.

cheers,
  Gerd
Markus Armbruster May 19, 2017, 5:54 a.m. UTC | #6
Thomas Huth <thuth@redhat.com> 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 <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.
>>>
>>> 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?
Thomas Huth May 19, 2017, 6:20 a.m. UTC | #7
On 19.05.2017 07:54, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> 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 <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.
>>>>
>>>> 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

Patch
diff mbox

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;
@@ -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;
 }