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 <mre...@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
signature.asc
Description: OpenPGP digital signature
