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

[Peter Maydell]

On 25 August 2015 at 16:25, Programmingkid <address@hidden> wrote:
> On Aug 25, 2015, at 8:42 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> On 08/24/2015 12:53 PM, Programmingkid wrote:
>>>> +/* 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.

Arbitrary limits are often a bad idea, especially when
they're easy to avoid, as here.

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

It's a glib function. Glib has a lot of useful utility functions
for this kind of thing (and the general idea of "have an
sprintf-alike which allocates the buffer for you" has been
around long before glib came along). Note that HACKING says that
you shouldn't use 'malloc' anyway, but 'malloc and then sprintf
into the buffer' is a particular antipattern that will get picked
up on in code review.

thanks
-- PMM