Re: [Qemu-devel] [PATCH V10 15/17] block: dump to buffer for bdrv_image_info_dump()

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-4-2 3:17, Eric Blake 写道:
> On 03/22/2013 08:19 AM, Wenchao Xia wrote:
>>    This allow hmp use this function, just like qemu-img.
>> It also returns a pointer now to make it easy to use.
>>
>
>>
>> -void bdrv_image_info_dump(ImageInfo *info)
>> +GCC_FMT_ATTR(3, 4)
>> +static void snprintf_tail(char **p_buf, int *p_size, const char *fmt, ...)
>
> Yuck.  I'm too worried that you are likely to cause truncation when you
> exceed the bounds of the fixed-width buffer.  And you can't argue that
> this is here to avoid malloc pressure, since...
>
>>
>> +#define IMAGE_INFO_BUF_SIZE (2048)
>>   static void dump_human_image_info_list(ImageInfoList *list)
>>   {
>>       ImageInfoList *elem;
>>       bool delim = false;
>> +    char *buf = g_malloc0(IMAGE_INFO_BUF_SIZE);
>
> ...you are doing a malloc for the original buffer in the first place.
> I'd much rather see use of g_string_append_printf or some similar glib
> interface that manages a dynamically-sized output buffer to begin with,
> than to attempt to force the output to fit in a fixed-width malloc'd buffer.
>
   I have considered it before, but here g_malloc0 always allocate
a fixed half page size which brings less fragment than dynamic
allocation according to string length, just as my comments in code:

+/* Use buf instead of asprintf to reduce memory fragility. */
+char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info)

  Personally I'd like to avoid asprintf since there would be many
times of reallocation for a single info dumping, not worthy I think,
and there will not be much trouble if string truncate is tipped.


-- 
Best Regards

Wenchao Xia