diff mbox

[v4,09/11] nbd: Add qemu-nbd -D for human-readable description

Message ID 1463006384-7734-10-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake May 11, 2016, 10:39 p.m. UTC 
The NBD protocol allows servers to advertise a human-readable
description alongside an export name during NBD_OPT_LIST.  Add
an option to pass through the user's string to the NBD client.

Doing this also makes it easier to test commit 200650d4, which
is the client counterpart of receiving the description.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  1 +
 nbd/nbd-internal.h  |  5 +++--
 nbd/server.c        | 34 ++++++++++++++++++++++++++--------
 qemu-nbd.c          | 12 +++++++++++-
 qemu-nbd.texi       |  5 ++++-
 5 files changed, 45 insertions(+), 12 deletions(-)

Comments

Daniel P. Berrangé May 12, 2016, 7:47 a.m. UTC | #1 
On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
> The NBD protocol allows servers to advertise a human-readable
> description alongside an export name during NBD_OPT_LIST.  Add
> an option to pass through the user's string to the NBD client.
> 
> Doing this also makes it easier to test commit 200650d4, which
> is the client counterpart of receiving the description.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h |  1 +
>  nbd/nbd-internal.h  |  5 +++--
>  nbd/server.c        | 34 ++++++++++++++++++++++++++--------
>  qemu-nbd.c          | 12 +++++++++++-
>  qemu-nbd.texi       |  5 ++++-
>  5 files changed, 45 insertions(+), 12 deletions(-)

> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 9f23343..923de74 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -79,9 +79,12 @@ Disconnect the device @var{dev}
>  Allow up to @var{num} clients to share the device (default @samp{1})
>  @item -t, --persistent
>  Don't exit on the last connection
> -@item -x NAME, --export-name=NAME
> +@item -x, --export-name=@var{name}

Why this change - that reads as saying that '-x' doesn't take any value
which is wrong IMHO

>  Set the NBD volume export name. This switches the server to use
>  the new style NBD protocol negotiation
> +@item -D, --description=@var{description}

Likewise this suggests -D doesn't take a value

> +Set the NBD volume export description, as a human-readable
> +string. Requires the use of @option{-x}
>  @item --tls-creds=ID
>  Enable mandatory TLS encryption for the server by setting the ID
>  of the TLS credentials object previously created with the --object

Regards,
Daniel
Eric Blake May 12, 2016, 3:38 p.m. UTC | #2 
On 05/12/2016 01:47 AM, Daniel P. Berrange wrote:
> On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
>> The NBD protocol allows servers to advertise a human-readable
>> description alongside an export name during NBD_OPT_LIST.  Add
>> an option to pass through the user's string to the NBD client.
>>
>> Doing this also makes it easier to test commit 200650d4, which
>> is the client counterpart of receiving the description.
>>

>> -@item -x NAME, --export-name=NAME
>> +@item -x, --export-name=@var{name}
> 
> Why this change - that reads as saying that '-x' doesn't take any value
> which is wrong IMHO

It's consistent with other options-with-arguments in the same file, such as:

@item -p, --port=@var{port}
@item -o, --offset=@var{offset}
@item -b, --bind=@var{iface}
@item -k, --socket=@var{path}

etc. Basically, we want to use this common escape hatch (see 'ls
--help', for example):

Mandatory arguments to long options are mandatory for short options too.

>> +@item -D, --description=@var{description}
> 
> Likewise this suggests -D doesn't take a value

If you don't like it, then we should have a separate patch for the
entire file should switch over to:

@item -D @var{description}, --description=@var{description}

style, where the mandatory option is listed for both option spellings
for every affected option.
Daniel P. Berrangé May 12, 2016, 3:51 p.m. UTC | #3 
On Thu, May 12, 2016 at 09:38:58AM -0600, Eric Blake wrote:
> On 05/12/2016 01:47 AM, Daniel P. Berrange wrote:
> > On Wed, May 11, 2016 at 04:39:42PM -0600, Eric Blake wrote:
> >> The NBD protocol allows servers to advertise a human-readable
> >> description alongside an export name during NBD_OPT_LIST.  Add
> >> an option to pass through the user's string to the NBD client.
> >>
> >> Doing this also makes it easier to test commit 200650d4, which
> >> is the client counterpart of receiving the description.
> >>
> 
> >> -@item -x NAME, --export-name=NAME
> >> +@item -x, --export-name=@var{name}
> > 
> > Why this change - that reads as saying that '-x' doesn't take any value
> > which is wrong IMHO
> 
> It's consistent with other options-with-arguments in the same file, such as:
> 
> @item -p, --port=@var{port}
> @item -o, --offset=@var{offset}
> @item -b, --bind=@var{iface}
> @item -k, --socket=@var{path}
> 
> etc. Basically, we want to use this common escape hatch (see 'ls
> --help', for example):
> 
> Mandatory arguments to long options are mandatory for short options too.

Ah, I didn't realize it was standard practice todo that - i personally
just historically include the arg value in both, but if QEMU doesn't
that's ok - consistency is more important.


Regards,
Daniel
diff mbox

Patch

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 134f117..3e2d76b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -107,6 +107,7 @@  BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
 void nbd_export_set_name(NBDExport *exp, const char *name);
+void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(NBDExport *exp,
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 3791535..035ead4 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -103,9 +103,10 @@  static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
     return nbd_wr_syncv(ioc, &iov, 1, 0, size, true);
 }

-static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
+static inline ssize_t write_sync(QIOChannel *ioc, const void *buffer,
+                                 size_t size)
 {
-    struct iovec iov = { .iov_base = buffer, .iov_len = size };
+    struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };

     return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
 }
diff --git a/nbd/server.c b/nbd/server.c
index 330ffd7..b8de8aa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -61,6 +61,7 @@  struct NBDExport {

     BlockBackend *blk;
     char *name;
+    char *description;
     off_t dev_offset;
     off_t size;
     uint16_t nbdflags;
@@ -128,7 +129,8 @@  static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)

 }

-static ssize_t nbd_negotiate_write(QIOChannel *ioc, void *buffer, size_t size)
+static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
+                                   size_t size)
 {
     ssize_t ret;
     guint watch;
@@ -224,11 +226,15 @@  static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)

 static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
 {
-    uint64_t magic, name_len;
+    uint64_t magic;
+    size_t name_len, desc_len;
     uint32_t opt, type, len;
+    const char *name = exp->name ? exp->name : "";
+    const char *desc = exp->description ? exp->description : "";

-    TRACE("Advertising export name '%s'", exp->name ? exp->name : "");
-    name_len = strlen(exp->name);
+    TRACE("Advertising export name '%s' description '%s'", name, desc);
+    name_len = strlen(name);
+    desc_len = strlen(desc);
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
         LOG("write failed (magic)");
@@ -244,18 +250,22 @@  static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
         LOG("write failed (reply type)");
         return -EINVAL;
     }
-    len = cpu_to_be32(name_len + sizeof(len));
+    len = cpu_to_be32(name_len + desc_len + sizeof(len));
     if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
         LOG("write failed (length)");
         return -EINVAL;
     }
     len = cpu_to_be32(name_len);
     if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
-        LOG("write failed (length)");
+        LOG("write failed (name length)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, exp->name, name_len) != name_len) {
-        LOG("write failed (buffer)");
+    if (nbd_negotiate_write(ioc, name, name_len) != name_len) {
+        LOG("write failed (name buffer)");
+        return -EINVAL;
+    }
+    if (nbd_negotiate_write(ioc, desc, desc_len) != desc_len) {
+        LOG("write failed (description buffer)");
         return -EINVAL;
     }
     return 0;
@@ -881,6 +891,12 @@  void nbd_export_set_name(NBDExport *exp, const char *name)
     nbd_export_put(exp);
 }

+void nbd_export_set_description(NBDExport *exp, const char *description)
+{
+    g_free(exp->description);
+    exp->description = g_strdup(description);
+}
+
 void nbd_export_close(NBDExport *exp)
 {
     NBDClient *client, *next;
@@ -890,6 +906,7 @@  void nbd_export_close(NBDExport *exp)
         client_close(client);
     }
     nbd_export_set_name(exp, NULL);
+    nbd_export_set_description(exp, NULL);
     nbd_export_put(exp);
 }

@@ -908,6 +925,7 @@  void nbd_export_put(NBDExport *exp)

     if (--exp->refcount == 0) {
         assert(exp->name == NULL);
+        assert(exp->description == NULL);

         if (exp->close) {
             exp->close(exp);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3ee19a2..1fa782a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -79,6 +79,7 @@  static void usage(const char *name)
 "  -t, --persistent          don't exit on the last connection\n"
 "  -v, --verbose             display extra debugging information\n"
 "  -x, --export-name=NAME    expose export by name\n"
+"  -D, --description=TEXT    with -x, also export a human-readable description\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
@@ -469,7 +470,7 @@  int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:D:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -495,6 +496,7 @@  int main(int argc, char **argv)
         { "verbose", no_argument, NULL, 'v' },
         { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", required_argument, NULL, 'x' },
+        { "description", required_argument, NULL, 'D' },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { NULL, 0, NULL, 0 }
@@ -514,6 +516,7 @@  int main(int argc, char **argv)
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     const char *export_name = NULL;
+    const char *export_description = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -677,6 +680,9 @@  int main(int argc, char **argv)
         case 'x':
             export_name = optarg;
             break;
+        case 'D':
+            export_description = optarg;
+            break;
         case 'v':
             verbose = 1;
             break;
@@ -903,7 +909,11 @@  int main(int argc, char **argv)
     }
     if (export_name) {
         nbd_export_set_name(exp, export_name);
+        nbd_export_set_description(exp, export_description);
         newproto = true;
+    } else if (export_description) {
+        error_report("Export description requires an export name");
+        exit(EXIT_FAILURE);
     }

     server_ioc = qio_channel_socket_new();
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9f23343..923de74 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -79,9 +79,12 @@  Disconnect the device @var{dev}
 Allow up to @var{num} clients to share the device (default @samp{1})
 @item -t, --persistent
 Don't exit on the last connection
-@item -x NAME, --export-name=NAME
+@item -x, --export-name=@var{name}
 Set the NBD volume export name. This switches the server to use
 the new style NBD protocol negotiation
+@item -D, --description=@var{description}
+Set the NBD volume export description, as a human-readable
+string. Requires the use of @option{-x}
 @item --tls-creds=ID
 Enable mandatory TLS encryption for the server by setting the ID
 of the TLS credentials object previously created with the --object