Re: [Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 31.08.2015 23:00, Eric Blake wrote:
> On 08/18/2015 05:10 PM, Max Reitz wrote:
>> Split the part which actually refreshes the BlockDriverState.filename
>> field off of bdrv_refresh_filename() into a more generic function
>> bdrv_filename(), which first calls bdrv_refresh_filename() and then
>> stores a qemu-usable filename into the given buffer instead of
>> BlockDriverState.filename.
>>
>> Since bdrv_refresh_filename() therefore no longer refreshes that field,
>> some calls to that function have to be replaced by calls to
>> bdrv_filename() "manually" refreshing the BDS filename field (this is
>> only temporary).
>>
>> Additionally, a wrapper function bdrv_filename_alloc() is added which
>> allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer
>> and returns it, since needing a temporary buffer for the filename is a
>> rather common pattern.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c               | 39 ++++++++++++++++++++++++++++++++-------
>>  block/blkverify.c     |  3 ++-
>>  block/quorum.c        |  2 +-
>>  include/block/block.h |  2 ++
>>  4 files changed, 37 insertions(+), 9 deletions(-)
>>
> 
>> +/* First refreshes exact_filename and full_open_options by calling
>> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
>> + * the target buffer. Otherwise, full_open_options is converted to a JSON
>> + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
>> + * put there.
>> + *
>> + * If sz > 0, the string put into the buffer will always be null-terminated.
>> + *
>> + * Returns @dest.
>> + */
>> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)
>> +{
> 
> How does one tell if 'sz' was large enough, vs. too short and therefore
> the snprintf() truncated the resulting string?  Would the code be any
> simpler if this always returned a freshly g_malloc'd string of the right
> length, rather than making the caller have to pre-allocate and guess
> whether the allocation was big enough?
> 
>> +    bdrv_refresh_filename(bs);
>> +
>> +    if (sz > INT_MAX) {
>> +        sz = INT_MAX;
>> +    }
>>  
>>      if (bs->exact_filename[0]) {
>> -        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
>> +        pstrcpy(dest, sz, bs->exact_filename);
>>      } else if (bs->full_open_options) {
>>          QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
>> -        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
>> -                 qstring_get_str(json));
>> +        snprintf(dest, sz, "json:%s", qstring_get_str(json));
>>          QDECREF(json);
>>      }
>> +
>> +    return dest;
> 
> In other words, I think it's very dangerous to use snprintf() without
> checking whether the result fit.

There are a couple of places in qemu which copy BDS.filename into a
pre-existing buffer, so I'd rather not just drop bdrv_filename() as it is.

I guess I'll just make it so that calling bdrv_filename() with a NULL
dest will result in the buffer being allocated.

Note however that there are a couple of places in qemu which rely on
filenames not exceeding PATH_MAX (by using PATH_MAX sized buffers for
holding them). Maybe we'll eventually get around to fix them, but right
now it's a limitation not introduced by this series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]