Re: [Qemu-devel] [PATCH 11/14] nbd/client: Add nbd_receive_export_list()

[Eric Blake]

On 12/7/18 4:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.12.2018 1:03, 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_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 hold more
> > members, along with a convenience function for freeing the list.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > ---

> > +
> > +/* Query details about a server's exports, then disconnect without
> > + * going into transmission phase. Return a count of the exports listed
> > + * in @info by the server, or -1 on error. Caller must free @info. */
> > +int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> > +                            const char *hostname, NBDExportInfo **info,
> > +                            Error **errp)
> > +{
> > +    int result;
> > +    int count = 0;
> > +    int i;
> > +    int rc;
> > +    int ret = -1;
> > +    NBDExportInfo *array = NULL;
> > +    uint32_t oldflags;
> > +    QIOChannel *sioc = NULL;
>
> it's a bit confusing that you use sioc name for the second produced ioc, when
> in nbd_client_init it's visa-versa.
>
> So, I assume that you mean Secure ioc, when in nbd_client_init it is Socket ioc.

I can s/sioc/outioc/ if that helps. Basically, the output ioc is the 
same as the original ioc for plaintext, and a secure wrapper socket for tls.

> > +    case 0: /* oldstyle, parse length and flags */
> > +        array = g_new0(NBDExportInfo, 1);
> > +        array->name = g_strdup("");
> > +        count = 1;
> > +
> > +        if (nbd_read(ioc, &array->size, sizeof(array->size), errp) < 0) {
> > +            error_prepend(errp, "Failed to read export length: ");
> > +            goto out;
> > +        }
> > +        array->size = be64_to_cpu(array->size);
> > +
> > +        if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
> > +            error_prepend(errp, "Failed to read export flags: ");
> > +            goto out;
> > +        }
> > +        oldflags = be32_to_cpu(oldflags);
> > +        if (oldflags & ~0xffff) {
> > +            error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> > +            goto out;
> > +        }
> > +        array->flags = oldflags;
>
>
> ^^^
> this is a common part of nbd_receive_export_list and nbd_receive_negotiate,
> can we move it to nbd_start_negotiate?

Not quite, but I could probably create yet another helper function.

> > +
> > + out:
> > +    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > +    qio_channel_close(ioc, NULL);
>
> interesting, why we don't close it (only shutdown) in other nbd code..

I presume you mean block/nbd-client.c:nbd_teardown_connection, which 
indeed oncly calls qio_channel_shutdown() followed by object_unref().  I 
think that unref'ing a channel implies a close, but if not, then that 
code is leaking an fd (shutdown ends the connection, but does not close 
resources).  I'll have to debug that, but it's independent of this patch.

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