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 <ebl...@redhat.com>
[...]
> @@ -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 <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
