Re: [Qemu-devel] [PATCH] qdev-monitor.c: Add device id generation

[Programmingkid]

On Aug 25, 2015, at 8:42 AM, Markus Armbruster wrote:

> My other reply is about design issues.  This one is about the actual
> code.  Until we get rough consensus on the former, the latter doesn't
> really matter, but here goes anyway.
> 
> Eric Blake <address@hidden> writes:
> 
>> On 08/24/2015 12:53 PM, Programmingkid wrote:
>>> Add device ID generation to each device if an ID isn't given.
>>> 
>>> Signed-off-by: John Arbuckle <address@hidden>
>>> 
>>> ---
>>> This patch can be tested by adding adding usb devices using the monitor.
>>> Start QEMU with the -usb option. Then go to the monitor and type
>>> "device_add usb-mouse". The ID of the device will be set to a number.
>>> Since QEMU will not allow an user to add a device with an ID set to a
>>> number, there is no chance for ID collisions. 
>>> 
>>> qdev-monitor.c |   24 ++++++++++++++++++------
>>> 1 files changed, 18 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index f9e2d62..98267c4 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -26,6 +26,10 @@
>>> #include "qapi/qmp/qerror.h"
>>> #include "qemu/config-file.h"
>>> #include "qemu/error-report.h"
>>> +#include <math.h>
>>> +
>>> +/* USB's max number of devices is 127. This number is 3 digits long. */
>>> +#define MAX_NUM_DIGITS_FOR_USB_ID 3
> 
> This limit makes no sense to me. 

The limit is used to decide how many characters the device_id string is going 
to have. 
Three digits would be 0 to 999 device ID's would be supported. I can't imagine
anyone spending the time to add that many devices.

> 
>>> 
>>> /*
>>>  * Aliases were a bad idea from the start.  Let's keep them
>>> @@ -574,17 +578,25 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>>> **errp)
>>>     id = qemu_opts_id(opts);
>>>     if (id) {
>>>         dev->id = id;
>>> +    } else { /* create an id for a device if none is provided */
>>> +        static int device_id_count;
>>> +
>>> +        /* Add one for '\0' character */
>>> +        char *device_id = (char *) malloc(sizeof(char) *
>>> +                                            MAX_NUM_DIGITS_FOR_USB_ID + 1);
>>> +        sprintf(device_id, "%d", device_id_count++);
>> 
>> g_strdup_printf() is a lot nicer about avoiding the risk of arbitrary
>> overflow...
>>> +        dev->id = (const char *) device_id;
>>> +
>>> +        /* if device_id_count >= 10^MAX_NUM_DIGITS_FOR_USB_ID */
>>> +        if (device_id_count >= pow(10, MAX_NUM_DIGITS_FOR_USB_ID)) {
>>> +            printf("Warning: Maximum number of device ID's 
>>> generated!\n\a");
>>> +            printf("Time for you to make your own device ID's.\n");
>> 
>> besides, printf() is probably the wrong way to do error reporting, and
>> we don't use \a BEL sequences anywhere else in qemu code.
>> 
>>> +        }
>>>     }
> 
> When device_id_count reaches the limit, you warn.  Next time around, you
> overrun the buffer.  Not good.

I could change it so next time around, only the warning is displayed. 

> 
> Eric is right, g_strdup_printf() is easier and safer.

If you say so. I have never heard of it myself.

> 
> I'd make the count 64 bits, so wrapping becomes vanishingly unlikely.

That big of a number seems unreasonably big. I can see the advantage of
using such a big number. Can QEMU even handle that many devices?

> 
>>> 
>>>     if (dev->id) {
>> 
>> This if would now be a dead check if your patch is applied.
>> 
>>>         object_property_add_child(qdev_get_peripheral(), dev->id,
>>>                                   OBJECT(dev), NULL);
>>> -    } else {
>>> -        static int anon_count;
>>> -        gchar *name = g_strdup_printf("device[%d]", anon_count++);
>>> -        object_property_add_child(qdev_get_peripheral_anon(), name,
>>> -                                  OBJECT(dev), NULL);
>>> -        g_free(name);
>>>     }
>> 
>> It looks like your goal was to move this code earlier, but you changed
>> enough aspects of it that I'm not sure what the right fix should be.
> 
> Drop the conditional, it's both useless and confusing after your patch.
Ok.

I'm thinking I will wait until the other maintainers and whoever else is 
interested,
say how they feel on the subject of generated ID's for devices before making
a new patch.