Thomas Huth <th...@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 <th...@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 <th...@redhat.com>
>>>>> Message-id: 1493883704-27604-1-git-send-email-th...@redhat.com
>>>>> Signed-off-by: Gerd Hoffmann <kra...@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?
