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

[Eric Blake]

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