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

[Programmingkid]

On Aug 25, 2015, at 11:33 AM, Peter Maydell wrote:

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

Knowing QEMU's limits can save the user from crashes and other problems. There 
is
only a finite amount of memory available to QEMU. 

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

Thank you very much for this info. Once the generated device ID
issue has been hammered down, I will make a new patch that
implements g_malloc and g_strdup_printf().