Re: [Qemu-devel] [PATCH 08/14] nbd/client: Refactor nbd_receive_list()

[Eric Blake]

On 12/6/18 8:18 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.12.2018 1:03, Eric Blake wrote:
> > Add some parameters to make this function reusable in upcoming
> > export listing, where we will want to capture the name and
> > description rather than compare against a user-supplied name.
> > No change in semantics to the existing caller.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > ---
> >    nbd/client.c | 66 +++++++++++++++++++++++++++++++++++++---------------
> >    1 file changed, 47 insertions(+), 19 deletions(-)

> > @@ -290,30 +298,49 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> > *want, bool *match,
> >            nbd_send_opt_abort(ioc);
> >            return -1;
> >        }
> > -    if (namelen != strlen(want)) {
> > -        if (nbd_drop(ioc, len, errp) < 0) {
> > -            error_prepend(errp,
> > -                          "failed to skip export name with wrong length: ");
> > -            nbd_send_opt_abort(ioc);
> > -            return -1;
> > +    if (want) {
> > +        if (namelen != strlen(want)) {
> > +            if (nbd_drop(ioc, len, errp) < 0) {
> > +                error_prepend(errp,
> > +                              "failed to skip export name with wrong length: 
> > ");
> > +                nbd_send_opt_abort(ioc);
> > +                return -1;
> > +            }
> > +            return 1;
> >            }
> > -        return 1;
> > +        assert(namelen < sizeof(array));
> > +    } else {
> > +        assert(nameout);
>
> this assert looks a bit redundant, if nameout is 0, next line will abort not 
> less clearly
>
> > +        *nameout = name = g_new(char, namelen + 1);
>
> We should check namelen <= NBD_MAX_NAME_SIZE before it, I think.

Why? We already validated that the overall option is not too large, and 
even if the resulting name from the server is larger than 
NBD_MAX_NAME_SIZE, it's still nice to report that name to the client for 
'qemu-nbd --list'.  And these days, we only call nbd_receive_list if a 
server lacked NBD_OPT_GO, which is getting rarer and rarer, so 
micro-optimizing a rare case to avoid a large malloc isn't going to make 
a noticeable difference.

> >        }
> >
> > -    assert(namelen < sizeof(name));
> >        if (nbd_read(ioc, name, namelen, errp) < 0) {
> >            error_prepend(errp, "failed to read export name: ");
> >            nbd_send_opt_abort(ioc);
> > +        if (!want) {
> > +            free(name);
>
> g_free
>
> > +        }
> >            return -1;
> >        }
> >        name[namelen] = '\0';
> >        len -= namelen;
> > -    if (nbd_drop(ioc, len, errp) < 0) {
> > +    if (!want) {
> > +        assert(description);
>
> NBD_MAX_NAME_SIZE

The description is not bound by NBD_MAX_NAME_SIZE.  The NBD protocol 
DOES document a maximum string size (4k), but we are (so far) not 
actually honoring that limit (our choice for NBD_MAX_NAME_SIZE is 256, 
which is smaller than the NBD protocol limit).

> > +        *description = g_new(char, len + 1);
> > +        if (nbd_read(ioc, *description, len, errp) < 0) {
> > +            error_prepend(errp, "failed to read export description: ");
> > +            nbd_send_opt_abort(ioc);
> > +            free(name);
> > +            free(*description);
>
> g_free
>
> > +            return -1;
> > +        }
> > +        (*description)[len] = '\0';
> > +    } else if (nbd_drop(ioc, len, errp) < 0) {
> >            error_prepend(errp, "failed to read export description: ");
> >            nbd_send_opt_abort(ioc);
> >            return -1;
> >        }
> > -    if (!strcmp(name, want)) {
> > +    if (want && !strcmp(name, want)) {
> >            *match = true;
> >        }
> >        return 1;
>
> one more thing: on fail path, you finally fill output name and description
> with freed pointers. I'd prefer to keep them unchanged in this case, however,
> it's a matter of taste.

Okay, I'll try and be more careful in v2 about not altering the callers 
pointers on failure.

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