Re: [RFC PATCH 10/18] qemu-storage-daemon: Add --chardev option

[Markus Armbruster <armbru@redhat.com>]

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)