[RFC,10/18] qemu-storage-daemon: Add --chardev option
diff mbox series

Message ID 20191017130204.16131-11-kwolf@redhat.com
State New
Headers show
Series
  • Add qemu-storage-daemon
Related show

Commit Message

Kevin Wolf Oct. 17, 2019, 1:01 p.m. UTC
This adds a --chardev option to the storage daemon that works the same
as the -chardev option of the system emulator.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-storage-daemon.c | 19 +++++++++++++++++++
 Makefile              |  2 +-
 2 files changed, 20 insertions(+), 1 deletion(-)

Comments

Markus Armbruster Nov. 8, 2019, 4:27 p.m. UTC | #1
Kevin Wolf <kwolf@redhat.com> writes:

> This adds a --chardev option to the storage daemon that works the same
> as the -chardev option of the system emulator.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-storage-daemon.c | 19 +++++++++++++++++++
>  Makefile              |  2 +-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index 099388f645..46e0a6ea56 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -26,6 +26,7 @@
>  
>  #include "block/block.h"
>  #include "block/nbd.h"
> +#include "chardev/char.h"
>  #include "crypto/init.h"
>  
>  #include "qapi/error.h"
> @@ -75,6 +76,9 @@ static void help(void)
>  "             [,driver specific parameters...]\n"
>  "                         configure a block backend\n"
>  "\n"
> +"  --chardev <options>    configure a character device backend\n"
> +"                         (see the qemu(1) man page for possible options)\n"

If pointing to the manual page was good enough for --help, we could save
ourselves a ton of trouble :)

> +"\n"
>  "  --export [type=]nbd,device=<node-name>[,name=<export-name>]\n"
>  "           [,writable=on|off][,bitmap=<name>]\n"
>  "                         export the specified block node over NBD\n"
> @@ -96,10 +100,13 @@ QEMU_HELP_BOTTOM "\n",
>  enum {
>      OPTION_OBJECT = 256,
>      OPTION_BLOCKDEV,
> +    OPTION_CHARDEV,
>      OPTION_NBD_SERVER,
>      OPTION_EXPORT,
>  };
>  
> +extern QemuOptsList qemu_chardev_opts;
> +
>  static QemuOptsList qemu_object_opts = {
>      .name = "object",
>      .implied_opt_name = "qom-type",
> @@ -130,6 +137,7 @@ static int process_options(int argc, char *argv[], Error **errp)
>          {"help", no_argument, 0, 'h'},
>          {"object", required_argument, 0, OPTION_OBJECT},
>          {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
> +        {"chardev", required_argument, 0, OPTION_CHARDEV},
>          {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
>          {"export", required_argument, 0, OPTION_EXPORT},
>          {"version", no_argument, 0, 'V'},
> @@ -189,6 +197,17 @@ static int process_options(int argc, char *argv[], Error **errp)
>                  qapi_free_BlockdevOptions(options);
>                  break;
>              }
> +        case OPTION_CHARDEV:
> +            {
> +                QemuOpts *opts = qemu_opts_parse(&qemu_chardev_opts,
> +                                                 optarg, true, &error_fatal);
> +                if (!qemu_chr_new_from_opts(opts, NULL, &error_fatal)) {
> +                    /* No error, but NULL returned means help was printed */
> +                    exit(EXIT_SUCCESS);
> +                }
> +                qemu_opts_del(opts);
> +                break;
> +            }

Differs from vl.c similar to --object [PATCH 02]:

* Options are processed left to right.  Good.

* You use &qemu_chardev_opts instead of qemu_opts_find().  Good.

* You use qemu_opts_parse() instead of qemu_opts_parse_noisily().  Only
  here I can actually point to a loss of help.

  For options where the argument is essentially a tagged union, an
  option argument "help" usually lists the tags, an argument "T,help"
  shows help on the parameters that go with tag T.

  --chardev is such an option.  Consider:

    $ qemu-system-x86_64 --chardev help
    Available chardev backend types: 
      ringbuf
    [...]
      tty
    $ qemu-storage-daemon --chardev help
    Available chardev backend types: 
      pty
    [...]
      tty

  Works the same, only the available backend types differ.  Intentional?

  Next:

    $ qemu-system-x86_64 --chardev tty,help
    chardev options:
      append=<bool (on/off)>
      backend=<str>
      chardev=<str>
      cols=<num>
    [...]
      width=<num>
    [Exit 1 ]

  The second help isn't very helpful, and the exit code is wrong.  Not
  this patch's problem.

    $ qemu-storage-daemon --chardev tty,help
    qemu-storage-daemon: Invalid parameter 'help'
    [Exit 1 ]

  This patch's problem :)

Also like --object, --chardev is paired with a QMP command, namely
chardev-add, and the two differ for historical reasons.  If we want to
give the storage daemon a QAPIfied command line from the start, we'll
have to decide how to address this issue.


>          case OPTION_NBD_SERVER:
>              {
>                  Visitor *v;
> diff --git a/Makefile b/Makefile
> index b913d4d736..0e3e98582d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -561,7 +561,7 @@ qemu-img.o: qemu-img-cmds.h
>  qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
> -qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) $(COMMON_LDADDS)
> +qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(chardev-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) $(COMMON_LDADDS)
>  
>  qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)

Patch
diff mbox series

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 099388f645..46e0a6ea56 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -26,6 +26,7 @@ 
 
 #include "block/block.h"
 #include "block/nbd.h"
+#include "chardev/char.h"
 #include "crypto/init.h"
 
 #include "qapi/error.h"
@@ -75,6 +76,9 @@  static void help(void)
 "             [,driver specific parameters...]\n"
 "                         configure a block backend\n"
 "\n"
+"  --chardev <options>    configure a character device backend\n"
+"                         (see the qemu(1) man page for possible options)\n"
+"\n"
 "  --export [type=]nbd,device=<node-name>[,name=<export-name>]\n"
 "           [,writable=on|off][,bitmap=<name>]\n"
 "                         export the specified block node over NBD\n"
@@ -96,10 +100,13 @@  QEMU_HELP_BOTTOM "\n",
 enum {
     OPTION_OBJECT = 256,
     OPTION_BLOCKDEV,
+    OPTION_CHARDEV,
     OPTION_NBD_SERVER,
     OPTION_EXPORT,
 };
 
+extern QemuOptsList qemu_chardev_opts;
+
 static QemuOptsList qemu_object_opts = {
     .name = "object",
     .implied_opt_name = "qom-type",
@@ -130,6 +137,7 @@  static int process_options(int argc, char *argv[], Error **errp)
         {"help", no_argument, 0, 'h'},
         {"object", required_argument, 0, OPTION_OBJECT},
         {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
+        {"chardev", required_argument, 0, OPTION_CHARDEV},
         {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
         {"export", required_argument, 0, OPTION_EXPORT},
         {"version", no_argument, 0, 'V'},
@@ -189,6 +197,17 @@  static int process_options(int argc, char *argv[], Error **errp)
                 qapi_free_BlockdevOptions(options);
                 break;
             }
+        case OPTION_CHARDEV:
+            {
+                QemuOpts *opts = qemu_opts_parse(&qemu_chardev_opts,
+                                                 optarg, true, &error_fatal);
+                if (!qemu_chr_new_from_opts(opts, NULL, &error_fatal)) {
+                    /* No error, but NULL returned means help was printed */
+                    exit(EXIT_SUCCESS);
+                }
+                qemu_opts_del(opts);
+                break;
+            }
         case OPTION_NBD_SERVER:
             {
                 Visitor *v;
diff --git a/Makefile b/Makefile
index b913d4d736..0e3e98582d 100644
--- a/Makefile
+++ b/Makefile
@@ -561,7 +561,7 @@  qemu-img.o: qemu-img-cmds.h
 qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
-qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) $(COMMON_LDADDS)
+qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(chardev-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) $(COMMON_LDADDS)
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)