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 <ebl...@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 <programmingk...@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.
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.
