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

[Eric Blake <eblake@redhat.com>]

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 <eblake@redhat.com>
>> ---
>>    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