[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev-monitor.c: Add device id generation
From: |
Programmingkid |
Subject: |
Re: [Qemu-devel] [PATCH] qdev-monitor.c: Add device id generation |
Date: |
Tue, 25 Aug 2015 11:50:18 -0400 |
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().
Re: [Qemu-devel] [PATCH] qdev-monitor.c: Add device id generation, Programmingkid, 2015/08/25
[Qemu-devel] Should we auto-generate IDs? (was: [PATCH] qdev-monitor.c: Add device id generation), Markus Armbruster, 2015/08/25