Re: [Qemu-block] [PATCH v4 05/10] qemu-io: allow specifying image as a set of options args

[Kevin Wolf]

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:
> Currently qemu-io allows an image filename to be passed on the
> command line, but unless using the JSON format, it does not have
> a way to set any options except the format eg
> 
>  qemu-io https://127.0.0.1/images/centos7.iso
>  qemu-io /home/berrange/demo.qcow2
> 
> This adds a --image-opts arg that indicates that the positional
> filename should be interpreted as a full option string, not
> just a filename.
> 
>  qemu-io --image-opts driver=http,url=https://127.0.0.1/images,sslverify=off
>  qemu-io --image-opts file=/home/berrange/demo.qcow2
> 
> This flag is mutually exclusive with the '-f' flag.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  qemu-io.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index d1432ea..51d8272 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -367,6 +367,7 @@ static void reenable_tty_echo(void)
>  
>  enum {
>      OPTION_OBJECT = 256,
> +    OPTION_IMAGE_OPTS = 257,
>  };
>  
>  static QemuOptsList qemu_object_opts = {
> @@ -397,6 +398,16 @@ static int object_create(void *opaque, QemuOpts *opts, 
> Error **errp)
>      return 0;
>  }
>  
> +static QemuOptsList file_opts = {
> +    .name = "file",
> +    .implied_opt_name = "file",
> +    .head = QTAILQ_HEAD_INITIALIZER(file_opts.head),
> +    .desc = {
> +        /* no elements => accept any params */
> +        { /* end of list */ }
> +    },
> +};
> +
>  int main(int argc, char **argv)
>  {
>      int readonly = 0;
> @@ -416,6 +427,7 @@ int main(int argc, char **argv)
>          { "cache", 1, NULL, 't' },
>          { "trace", 1, NULL, 'T' },
>          { "object", 1, NULL, OPTION_OBJECT },
> +        { "image-opts", 0, NULL, OPTION_IMAGE_OPTS },
>          { NULL, 0, NULL, 0 }
>      };
>      int c;
> @@ -424,6 +436,7 @@ int main(int argc, char **argv)
>      Error *local_error = NULL;
>      QDict *opts = NULL;
>      QemuOpts *qopts = NULL;
> +    bool imageOpts = false;
>  
>  #ifdef CONFIG_POSIX
>      signal(SIGPIPE, SIG_IGN);
> @@ -492,6 +505,9 @@ int main(int argc, char **argv)
>                  exit(1);
>              }
>              break;
> +        case OPTION_IMAGE_OPTS:
> +            imageOpts = true;
> +            break;
>          default:
>              usage(progname);
>              exit(1);
> @@ -534,7 +550,23 @@ int main(int argc, char **argv)
>          flags |= BDRV_O_RDWR;
>      }
>  
> -    if ((argc - optind) == 1) {
> +    if (imageOpts) {
> +        char *file;
> +        qopts = qemu_opts_parse_noisily(&file_opts, argv[optind], false);
> +        if (!qopts) {
> +            exit(1);
> +        }
> +        if (opts) {
> +            error_report("--image-opts and -f are mutually exclusive");
> +            exit(1);
> +        }
> +        file = g_strdup(qemu_opt_get(qopts, "file"));
> +        qemu_opt_unset(qopts, "file");

I'm not sure if special casing "file" is a good idea. I think you're
doing it in order to support something that looks like -drive, but it's
not the same anyway - all of the options that are parsed in drive_new()
or blockdev_init() aren't supported.

We could change things so that qemu-io actually goes through
drive_new() or blockdev_init(), at the cost of forbidding node
references for the "file" options like -drive unfortunately does. Or we
can require that you use actual bdrv_open() options and specify
file.filename=... for raw-posix.

But I would like to avoid creating a new type of option parsing with yet
another behaviour.

> +        opts = qemu_opts_to_qdict(qopts, NULL);
> +        qemu_opts_reset(&file_opts);
> +        openfile(file, flags, opts);
> +        g_free(file);
> +    } else if ((argc - optind) == 1) {
>          openfile(argv[optind], flags, opts);
>      }
>      command_loop();

Kevin