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

[Markus Armbruster <armbru@redhat.com>]

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 <eblake@redhat.com> 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 <programmingkidx@gmail.com>
>> 
>> ---
>> 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.

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

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

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

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