On Aug 25, 2015, at 11:33 AM, Peter Maydell wrote: > On 25 August 2015 at 16:25, Programmingkid <programmingk...@gmail.com> wrote: >> On Aug 25, 2015, at 8:42 AM, Markus Armbruster wrote: >>> Eric Blake <ebl...@redhat.com> 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().