Re: [PATCH 2/2] block: Call attention to truncation of long NBD exports

[Eric Blake <eblake@redhat.com>]

On 6/10/20 4:24 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2020 21:26, Eric Blake wrote:
>> Commit 93676c88 relaxed our NBD client code to request export names up
>> to the NBD protocol maximum of 4096 bytes without NUL terminator, even
>> though the block layer can't store anything longer than 4096 bytes
>> including NUL terminator for display to the user.  Since this means
>> there are some export names where we have to truncate things, we can
>> at least try to make the truncation a bit more obvious for the user.
>> Note that in spite of the truncated display name, we can still
>> communicate with an NBD server using such a long export name; this was
>> deemed nicer than refusing to even connect to such a server (since the
>> server may not be under our control, and since determining our actual
>> length limits gets tricky when nbd://host:port/export and
>> nbd+unix:///export?socket=/path are themselves variable-length
>> expansions beyond the export name but count towards the block layer
>> name length).
>>
>> Reported-by: Xueqiang Wei <xuwei@redhat.com>
>> Fixes: https://bugzilla.redhat.com/1843684
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> ---
>>   block.c     |  7 +++++--
>>   block/nbd.c | 21 +++++++++++++--------
>>   2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 8416376c9b71..6dbcb7e083ea 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6809,8 +6809,11 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>           pstrcpy(bs->filename, sizeof(bs->filename), 
>> bs->exact_filename);
>>       } else {
>>           QString *json = 
>> qobject_to_json(QOBJECT(bs->full_open_options));
>> -        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
>> -                 qstring_get_str(json));
>> +        if (snprintf(bs->filename, sizeof(bs->filename), "json:%s",
>> +                     qstring_get_str(json)) >= sizeof(bs->filename)) {
>> +            /* Give user a hint if we truncated things. */
>> +            strcpy(bs->filename + sizeof(bs->filename) - 4, "...");
>> +        }
> 
> Is  4096 really enough for json in normal cases?

By its very nature, a json string tends be longer than a counterpart URI 
string representing the same information (when such an explicit name 
exists) because of the extra characters burned in adding "key":value 
pairs wrapping the data that was compact in explicit form.  But 4k is 
still quite a lot, and the only cases I've seen where names don't fit in 
JSON form is where the user was explicitly trying to break things with 
corner-case testing, rather than what you get with day-to-day use.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org