Re: [Qemu-devel] [PATCH 06/14] nbd/client: Move export name into NBDExportInfo

[Vladimir Sementsov-Ogievskiy]

01.12.2018 1:03, Eric Blake wrote:
> Refactor the 'name' parameter of nbd_receive_negotiate() from
> being a separate parameter into being part of the in-out 'info'.
> This also spills over to a simplification of nbd_opt_go().
> 
> The main driver for this refactoring is that an upcoming patch
> would like to add support to qemu-nbd to list information about
> all exports available on a server, where the name(s) will be
> provided by the server instead of the client.  But another benefit
> is that we can now allow the client to explicitly specify the
> empty export name "" even when connecting to an oldstyle server
> (even if qemu is no longer such a server after commit 7f7dfe2a).
> 
> Signed-off-by: Eric Blake <address@hidden>

[...]

> @@ -877,8 +874,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name,
>       } else if (magic == NBD_CLIENT_MAGIC) {
>           uint32_t oldflags;
> 
> -        if (name) {
> -            error_setg(errp, "Server does not support export names");
> +        if (*info->name) {
> +            error_setg(errp, "Server does not support non-empty export 
> names");

hm, old message is a bit nearer to nbd spec, the new may be a bit more
understandable in context of current qemu-nbd help:
"-x, --export-name=NAME    expose export by name (default is empty string)"

so, I'm OK with either one.

>               goto fail;
>           }
>           if (tlscreds) {
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 866e64779f1..c57053a0795 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -263,7 +263,7 @@ static void *show_parts(void *arg)
>   static void *nbd_client_thread(void *arg)
>   {
>       char *device = arg;
> -    NBDExportInfo info = { .request_sizes = false, };
> +    NBDExportInfo info = { .request_sizes = false, .name = g_strdup("") };
>       QIOChannelSocket *sioc;
>       int fd;
>       int ret;
> @@ -278,7 +278,7 @@ static void *nbd_client_thread(void *arg)
>           goto out;
>       }
> 
> -    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL,
> +    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc),
>                                   NULL, NULL, NULL, &info, &local_error);
>       if (ret < 0) {
>           if (local_error) {
> @@ -317,6 +317,7 @@ static void *nbd_client_thread(void *arg)
>       }
>       close(fd);
>       object_unref(OBJECT(sioc));
> +    free(info.name);

g_free

>       kill(getpid(), SIGTERM);
>       return (void *) EXIT_SUCCESS;
> 
> @@ -325,6 +326,7 @@ out_fd:
>   out_socket:
>       object_unref(OBJECT(sioc));
>   out:
> +    free(info.name);

and here

>       kill(getpid(), SIGTERM);
>       return (void *) EXIT_FAILURE;
>   }
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 5e1d4afe8e6..289337d0dc3 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -15,7 +15,7 @@ nbd_opt_meta_reply(const char *context, uint32_t id) 
> "Received mapping of contex
>   nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving 
> negotiation tlscreds=%p hostname=%s"
>   nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
>   nbd_receive_negotiate_server_flags(uint32_t globalflags) "Global flags are 
> 0x%" PRIx32
> -nbd_receive_negotiate_default_name(void) "Using default NBD export name \"\""
> +nbd_receive_negotiate_name(const char *name) "Requesting NBD export name 
> \"%s\""

Other trace points prefers to use single quotes with export name (or without 
quotes),
this may be changed too. Oh, too deep nit-picking o_0

>   nbd_receive_negotiate_size_flags(uint64_t size, uint16_t flags) "Size is %" 
> PRIu64 ", export flags 0x%" PRIx16
>   nbd_init_set_socket(void) "Setting NBD socket"
>   nbd_init_set_block_size(unsigned long block_size) "Setting block size to 
> %lu"
> 

with fixed s/free/g_free:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


-- 
Best regards,
Vladimir