diff mbox

[v4,05/10] qemu-io: allow specifying image as a set of options args

Message ID 1453815262-13440-6-git-send-email-berrange@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel P. Berrangé Jan. 26, 2016, 1:34 p.m. UTC
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 <berrange@redhat.com>
---
 qemu-io.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Comments

Kevin Wolf Jan. 27, 2016, 2:26 p.m. UTC | #1
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 <berrange@redhat.com>
> ---
>  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
Daniel P. Berrangé Feb. 2, 2016, 10:13 a.m. UTC | #2
On Wed, Jan 27, 2016 at 03:26:51PM +0100, Kevin Wolf wrote:
> Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:
> > -    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.

Mostly this special casing of 'file' was based on my confusion /
misunderstanding of the recommended best API usage around bdrv_open.
I thought I was being compatible with -drive from the system emulators
by doing this, but you point out I'm not actually doing it properly.

Lets just keep this simple and use bdrv_open() options explicitly
since IIUC that looks like the "best practice" approach these days


Regards,
Daniel
diff mbox

Patch

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");
+        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();