Re: [Qemu-devel] [PATCH v2 18/22] nbd/client: Add nbd_receive_export_list()

[Eric Blake <eblake@redhat.com>]

On 12/15/18 9:42 AM, Richard W.M. Jones wrote:
> On Sat, Dec 15, 2018 at 07:53:20AM -0600, Eric Blake wrote:
>> We want to be able to detect whether a given qemu NBD server is
>> exposing the right export(s) and dirty bitmaps, at least for
>> regression testing.  We could use 'nbd-client -l' from the upstream
>> NBD project to list exports, but it's annoying to rely on
>> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
>> know about all of the qemu NBD extensions.  Thus, we plan on adding
>> a new mode to qemu-nbd that merely sniffs all possible information
>> from the server during handshake phase, then disconnects and dumps
>> the information.
>>
>> This patch adds the low-level client code for grabbing the list
>> of exports.  It benefits from the recent refactoring patches, as
>> well as a minor tweak of changing nbd_opt_go() to nbd_opt_info_or_go(),
>> in order to share as much code as possible when it comes to doing
>> validation of server replies.  The resulting information is stored
>> in an array of NBDExportInfo which has been expanded to any
>> description string, along with a convenience function for freeing
>> the list.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---

>> +        while (1) {
>> +            char *name;
>> +            char *desc;
>> +
>> +            rc = nbd_receive_list(ioc, &name, &desc, errp);
>> +            if (rc < 0) {
>> +                goto out;
>> +            } else if (rc == 0) {
>> +                break;
>> +            }
>> +            array = g_renew(NBDExportInfo, array, ++count);
>> +            memset(&array[count - 1], 0, sizeof(*array));
>> +            array[count - 1].name = name;
>> +            array[count - 1].description = desc;
>> +            array[count - 1].structured_reply = result == 3;
>> +        }
> 
> Do we care about limiting ‘count’ to some reasonable value here?

Maybe.

> 
> I tried to look at the protocol document to see if there's a limit on
> the number of exports that a server can have, but if there is I cannot
> find it.  I don't know how much we care about malicious NBD servers --
> mostly I'm interested in malicious NBD clients :-)

You brought up the same question earlier in the series, and we already 
have a pre-existing repeat of the situation with NBD_OPT_LIST for 
servers that lack NBD_OPT_GO.

A client can always hang up on a server that it doesn't like; so maybe 
we could set our own limit - but what is a good limit?  An NBD server 
set up to export every file in a given directory, where that directory 
has over 1k files, doesn't seem too far-fetched.  Having 1000 contexts 
available in NBD_OPT_LIST_META_CONTEXT, on the other hand, is less likely.

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