Skip to contentSkip to conversation

Make qemu-img/qemu-nbd/qemu-io CLI more flexible

PATCH Superseded Daniel P. Berrange 2 participants 19 messages Jan 26, 2016 – Feb 2, 2016

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v4 cover Make qemu-img/qemu-nbd/qemu-io CLI more flexible

This series of patches expands the syntax of the qemu-img, qemu-nbd and qemu-io commands to make them more flexible.

v0: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04014.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04354.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03381.html

First all three gain a --object parameter, which allows instantiation of user creatable object types. The immediate use case is to allow for creation of the 'secret' object type to pass passwords for curl, iscsi and rbd drivers. For qemu-nbd this will also be needed to create TLS certificates for encryption support.

Then all three gain a '--image-opts' parameter which causes the positional filenames to be interepreted as option strings rather tha nplain filenames. This avoids the need to use the JSON syntax, or to add custom CLI args for each block backend option that exists. The immediate use case is to allow the user to specify the ID of the 'secret' object they just created.

Finally, there are a few small cleanup patches

The first 4 patches in this series are a pre-requisite for 3 other series

- Support for TLS in NBD
- Support for secrets for passwd auth in curl, rbd, iscsi (fixes a CVE issue in libvirt)
- Support for LUKS encryption passwords

Hopefully the --object patches are fairly uncontroversial and can be merged soon. The latter patches for --image-opts are very nice to have, but not a hard blocker right now since the 'json:{....}' syntax can be used until they are merged.

Changed in v4:

- Fix error reporting when object_create fails

Changed in v3:

- Rebase to resolve with conflicts against recently merged code
- Remove use of errx()

Changed in v2:

- Share more common code in qom/object_interfaces.c to avoid duplicating so much of 'object_create' in each command
- Remove previously added '--source optstring' parameter which replaced the positional filenames, in favour of keeping the positional filenames but using a --image-opts boolean arg to change their interpretation
- Added docs for --image-opts to qemu-img man page
- Use printf instead of echo -n in examples
- Line wrap help string based on user terminal width not source code width
- Update qemu-nbd/qemu-io to use constants for options
- Update qemu-nbd to avoid overlapping option values

Daniel P. Berrange (10):
qom: add helpers for UserCreatable object types
qemu-img: add support for --object command line arg
qemu-nbd: add support for --object command line arg
qemu-io: add support for --object command line arg
qemu-io: allow specifying image as a set of options args
qemu-nbd: allow specifying image as a set of options args
qemu-img: allow specifying image as a set of options args
qemu-nbd: don't overlap long option values with short options
qemu-nbd: use no_argument/required_argument constants
qemu-io: use no_argument/required_argument constants

 hmp.c                           |  52 +---
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  48 ++++
 qemu-img-cmds.hx                |  44 +--
 qemu-img.c                      | 588 +++++++++++++++++++++++++++++++++++++---
 qemu-img.texi                   |  14 +
 qemu-io.c                       | 114 +++++++-
 qemu-nbd.c                      | 150 ++++++++--
 qemu-nbd.texi                   |   6 +
 qmp.c                           |  76 +-----
 qom/object_interfaces.c         | 139 ++++++++++
 vl.c                            |  48 +---
 12 files changed, 1029 insertions(+), 253 deletions(-)
Signature · 1 line
2.5.0
#1 lore raw

PATCH v4 1/10 qom: add helpers for UserCreatable object types

The QMP monitor code has two helper methods object_add and qmp_object_del that are called from several places in the code (QMP, HMP and main emulator startup).

The HMP and main emulator startup code also share further logic that extracts the qom-type & id values from a qdict.

We soon need to use this logic from qemu-img, qemu-io and qemu-nbd too, but don't want those to depend on the monitor, nor do we want to duplicate the code.

To avoid this, move some code out of qmp.c and hmp.c adding 3 new methods to qom/object_interfaces.c

- user_creatable_add - takes a QDict holding a full object definition & instantiates it
- user_creatable_add_type - takes an ID, type name, and QDict holding object properties & instantiates it
- user_creatable_del - takes an ID and deletes the corresponding object

The existing code is updated to use these new methods.

---
 hmp.c                           |  52 ++++-----------
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  48 ++++++++++++++
 qmp.c                           |  76 ++--------------------
 qom/object_interfaces.c         | 139 ++++++++++++++++++++++++++++++++++++++++
 vl.c                            |  48 ++++----------
 6 files changed, 216 insertions(+), 150 deletions(-)
hmp.c +11 -41
index 54f2620..95930b0 100644
@@ -29,6 +29,7 @@
 #include "qapi/string-output-visitor.h"
 #include "qapi/util.h"
 #include "qapi-visit.h"
+#include "qom/object_interfaces.h"
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
@@ -1652,58 +1653,27 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    Error *err_end = NULL;
     QemuOpts *opts;
-    char *type = NULL;
-    char *id = NULL;
-    void *dummy = NULL;
     OptsVisitor *ov;
-    QDict *pdict;
+    Object *obj = NULL;
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
-        goto out;
+        hmp_handle_error(mon, &err);
+        return;
     }
 
     ov = opts_visitor_new(opts);
-    pdict = qdict_clone_shallow(qdict);
-
-    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
-    if (err) {
-        goto out_clean;
-    }
-
-    qdict_del(pdict, "qom-type");
-    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
-    if (err) {
-        goto out_end;
-    }
+    obj = user_creatable_add(qdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    qemu_opts_del(opts);
 
-    qdict_del(pdict, "id");
-    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
     if (err) {
-        goto out_end;
+        hmp_handle_error(mon, &err);
     }
-
-    object_add(type, id, pdict, opts_get_visitor(ov), &err);
-
-out_end:
-    visit_end_struct(opts_get_visitor(ov), &err_end);
-    if (!err && err_end) {
-        qmp_object_del(id, NULL);
+    if (obj) {
+        object_unref(obj);
     }
-    error_propagate(&err, err_end);
-out_clean:
-    opts_visitor_cleanup(ov);
-
-    QDECREF(pdict);
-    qemu_opts_del(opts);
-    g_free(id);
-    g_free(type);
-    g_free(dummy);
-
-out:
-    hmp_handle_error(mon, &err);
 }
 
 void hmp_getfd(Monitor *mon, const QDict *qdict)
@@ -1933,7 +1903,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
     const char *id = qdict_get_str(qdict, "id");
     Error *err = NULL;
 
-    qmp_object_del(id, &err);
+    user_creatable_del(id, &err);
     hmp_handle_error(mon, &err);
 }
 
include/monitor/monitor.h +0 -3
index 91b95ae..aa0f373 100644
@@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
 
-void object_add(const char *type, const char *id, const QDict *qdict,
-                Visitor *v, Error **errp);
-
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp);
include/qom/object_interfaces.h +48 -0

58 lines not on this page · raw message

qmp.c +7 -69

99 lines not on this page · raw message

qom/object_interfaces.c +139 -0

145 lines not on this page · raw message

vl.c +11 -37

63 lines not on this page · raw message

Signature · 1 line
2.5.0
#2 lore raw

PATCH v4 2/10 qemu-img: add support for --object command line arg

In reply to Daniel P. Berrange

Allow creation of user creatable object types with qemu-img via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
      ...other info args...
---
 qemu-img-cmds.hx |  44 ++++-----
 qemu-img.c       | 284 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 qemu-img.texi    |   8 ++
 3 files changed, 306 insertions(+), 30 deletions(-)
qemu-img-cmds.hx +22 -22
index 9567774..5bb1de7 100644
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [--object objectdef] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object objectdef] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [-f fmt] [-o options] filename [size]")
+    "create [-q] [--object objectdef] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object objectdef] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
+    "commit [-q] [--object objectdef] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
 STEXI
-@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object objectdef] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
+    "compare [--object objectdef] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
 STEXI
-@item compare [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object objectdef] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object objectdef] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
-    "info [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [-f fmt] [--output=ofmt] [--backing-chain] filename")
 STEXI
-@item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object objectdef] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [-f fmt] [--output=ofmt] filename")
 STEXI
-@item map [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+@item map [--object objectdef] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
 ETEXI
 
 DEF("snapshot", img_snapshot,
-    "snapshot [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
+    "snapshot [--object objectdef] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
-@item snapshot [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
+@item snapshot [--object objectdef] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [--object objectdef] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
-    "resize [-q] filename [+ | -]size")
+    "resize [--object objectdef] [-q] filename [+ | -]size")
 STEXI
-@item resize [-q] @var{filename} [+ | -]@var{size}
+@item resize [--object objectdef] [-q] @var{filename} [+ | -]@var{size}
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [--object objectdef] [-p] [-q] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [--object objectdef] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
qemu-img.c +276 -8

561 lines not on this page · raw message

qemu-img.texi +8 -0
index 55c6be3..da93272 100644
@@ -24,6 +24,14 @@ Command parameters:
 @table @var
 @item filename
  is a disk image filename
+
+@item --object @var{objectdef}
+
+is a QEMU user creatable object definition. See the @code{qemu(1)} manual
+page for a description of the object properties. The only object type that
+it makes sense to define is the @code{secret} object, which is used to
+supply passwords and/or encryption keys.
+
 @item fmt
 is the disk image format. It is guessed automatically in most cases. See below
 for a description of the supported disk formats.
Signature · 1 line
2.5.0
#3 lore raw

Re: [Qemu-devel] [PATCH v4 02/10] qemu-img: add support for --object command line arg

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 16 lines

Allow creation of user creatable object types with qemu-img via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
      ...other info args...
qemu-img-cmds.hx +0 -0
index 9567774..5bb1de7 100644

qemu-img-cmds.hx STEXI · line 16

Quoted 9 lines
qemu-img-cmds.hx +2 -2
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [--object objectdef] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object objectdef] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI

s/objectdef/@var{objectdef}/ (in each command)

qemu-img-cmds.hx static void QEMU_NORETURN help(void) · line 107

Quoted 11 lines
qemu-img-cmds.hx +4 -0
@@ -94,6 +98,10 @@ static void QEMU_NORETURN help(void)
            "\n"
            "Command parameters:\n"
            "  'filename' is a disk image filename\n"
+           "  'objectdef' is a QEMU user creatable object definition. See the @code{qemu(1)}\n"
+           "    manual page for a description of the object properties. The common object\n"
+           "    type that it makes sense to define is the @code{secret} object, which is used\n"
+           "    to supply passwords and/or encryption keys.\n"
            "  'fmt' is the disk image format. It is guessed automatically in most cases\n"
            "  'cache' is the cache mode used to write the output disk image, the valid\n"
            "    options are: 'none', 'writeback' (default, except for convert), 'writethrough',\n"

This one in contrast is printed literally on stdout, so using @code{} is not a great idea.

Quoted 36 lines
qemu-img-cmds.hx +28 -0
@@ -154,6 +162,34 @@ static void QEMU_NORETURN help(void)
     exit(EXIT_SUCCESS);
 }
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+
+    user_creatable_add(pdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    QDECREF(pdict);
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}
+
 static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
 {
     int ret = 0;

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 312

Quoted 4 lines
qemu-img-cmds.hx +1 -0
@@ -273,9 +309,17 @@ static int img_create(int argc, char **argv)
     char *options = NULL;
     Error *local_err = NULL;
     bool quiet = false;
+    QemuOpts *opts;

There are places where we declare variables only used by a specific option locally with a new block after the case label. This could be another one for which it is appropriate - it's not used after the option parsing any more (and it can't be used there because it may still be uninitialised).

Quoted 14 lines
qemu-img-cmds.hx +8 -1
     for(;;) {
-        c = getopt(argc, argv, "F:b:f:he6o:q");
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"object", required_argument, 0, OPTION_OBJECT},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "F:b:f:he6o:q",
+                        long_options, &option_index);
         if (c == -1) {
             break;
         }

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 366

Quoted 6 lines
qemu-img-cmds.hx +3 -0
@@ -317,6 +361,13 @@ static int img_create(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case OPTION_OBJECT:
+            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                           optarg, true);

Any reason for using qemu_add_opts() to register the opts list and then finding it again by name instead of just using &qemu_object_opts here?

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 369

Quoted 3 lines
qemu-img-cmds.hx +3 -0
+            if (!opts) {
+                exit(1);
+            }

You seem to introduce a lot of exit(1) calls even where the surrounding code prefers return 1.

Also, for other patches Eric has been asking to use EXIT_FAILURE instead of 1 in new code, and I think that makes sense, too.

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 372

Quoted 3 lines
qemu-img-cmds.hx +1 -0
+            break;
         }
     }

Kevin

#4 lore raw

Re: [Qemu-devel] [PATCH v4 02/10] qemu-img: add support for --object command line arg

Quoted 19 lines · Kevin Wolf

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 18 lines

Allow creation of user creatable object types with qemu-img via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
      ...other info args...
qemu-img-cmds.hx +0 -0
index 9567774..5bb1de7 100644
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
Quoted 9 lines

qemu-img-cmds.hx STEXI · line 16

Quoted 7 lines
qemu-img-cmds.hx +2 -2
 DEF("check", img_check,
-    "check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [--object objectdef] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object objectdef] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI

s/objectdef/@var{objectdef}/ (in each command)

Ok, will fix.

Quoted 6 lines
Quoted 6 lines
qemu-img-cmds.hx +2 -0
@@ -94,6 +98,10 @@ static void QEMU_NORETURN help(void)
            "\n"
            "Command parameters:\n"
            "  'filename' is a disk image filename\n"
+           "  'objectdef' is a QEMU user creatable object definition. See the @code{qemu(1)}\n"
+           "    manual page for a description of the object properties. The common object\n"
Quoted 8 lines

qemu-img-cmds.hx static void QEMU_NORETURN help(void) · line 107

Quoted 5 lines
qemu-img-cmds.hx +2 -0
+           "    type that it makes sense to define is the @code{secret} object, which is used\n"
+           "    to supply passwords and/or encryption keys.\n"
            "  'fmt' is the disk image format. It is guessed automatically in most cases\n"
            "  'cache' is the cache mode used to write the output disk image, the valid\n"
            "    options are: 'none', 'writeback' (default, except for convert), 'writethrough',\n"

This one in contrast is printed literally on stdout, so using @code{} is not a great idea.

Opps, yes.

Quoted 6 lines
Quoted 6 lines

static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)

 {
     int ret = 0;
qemu-img-cmds.hx +0 -0
@@ -273,9 +309,17 @@ static int img_create(int argc, char **argv)
     char *options = NULL;
     Error *local_err = NULL;
Quoted 8 lines

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 312

Quoted 2 lines
qemu-img-cmds.hx +1 -0
     bool quiet = false;
+    QemuOpts *opts;

There are places where we declare variables only used by a specific option locally with a new block after the case label. This could be another one for which it is appropriate - it's not used after the option parsing any more (and it can't be used there because it may still be uninitialised).

Ok, will put the decl inside the switch case that uses it.

Quoted 14 lines
Quoted 14 lines
qemu-img-cmds.hx +8 -1
-        c = getopt(argc, argv, "F:b:f:he6o:q");
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"object", required_argument, 0, OPTION_OBJECT},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "F:b:f:he6o:q",
+                        long_options, &option_index);
         if (c == -1) {
             break;
         }
@@ -317,6 +361,13 @@ static int img_create(int argc, char **argv)
         case 'q':
Quoted 8 lines

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 366

Quoted 5 lines
qemu-img-cmds.hx +3 -0
             quiet = true;
             break;
+        case OPTION_OBJECT:
+            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                           optarg, true);

Any reason for using qemu_add_opts() to register the opts list and then finding it again by name instead of just using &qemu_object_opts here?

No reason at all really, other than copying the pattern from vl.c. I'll change to use &qemu_object_opts instead.

Quoted 10 lines

qemu-img-cmds.hx static int img_create(int argc, char **argv) · line 369

Quoted 4 lines
qemu-img-cmds.hx +3 -0
+            if (!opts) {
+                exit(1);
+            }

You seem to introduce a lot of exit(1) calls even where the surrounding code prefers return 1.

Also, for other patches Eric has been asking to use EXIT_FAILURE instead of 1 in new code, and I think that makes sense, too.

For added fun several img_XXX commands are slightly different. I'll go through and make each one consistent with surrounding code, either a return, or a goto as applicable.

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#5 lore raw

PATCH v4 3/10 qemu-nbd: add support for --object command line arg

In reply to Daniel P. Berrange

Allow creation of user creatable object types with qemu-nbd via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
      ...other nbd args...
---
 qemu-nbd.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-nbd.texi |  6 ++++++
 2 files changed, 60 insertions(+)
qemu-nbd.c +54 -0
index ede4a54..8e5d36c 100644
@@ -23,9 +23,12 @@
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/opts-visitor.h"
+#include "qom/object_interfaces.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -44,6 +47,7 @@
 #define QEMU_NBD_OPT_AIO           2
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
 static int verbose;
@@ -77,6 +81,9 @@ static void usage(const char *name)
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
+"General purpose options:\n"
+"  --object type,id=ID,...   define an object such as 'secret' for providing\n"
+"                            passwords and/or encryption keys\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -374,6 +381,35 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+
+    user_creatable_add(pdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    QDECREF(pdict);
+
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -411,6 +447,7 @@ int main(int argc, char **argv)
         { "format", 1, NULL, 'f' },
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
+        { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -428,6 +465,7 @@ int main(int argc, char **argv)
     Error *local_err = NULL;
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
+    QemuOpts *opts;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -436,6 +474,8 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);
     qemu_init_exec_dir(argv[0]);
 
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
@@ -588,6 +628,13 @@ int main(int argc, char **argv)
             usage(argv[0]);
             exit(0);
             break;
+        case QEMU_NBD_OPT_OBJECT:
+            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                           optarg, true);
+            if (!opts) {
+                exit(1);
+            }
+            break;
         case '?':
             error_report("Try `%s --help' for more information.", argv[0]);
             exit(EXIT_FAILURE);
@@ -600,6 +647,13 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("object"),
+                          object_create,
+                          NULL, &local_err)) {
+        error_report_err(local_err);
+        exit(1);
+    }
+
     if (disconnect) {
         fd = open(argv[optind], O_RDWR);
         if (fd < 0) {
qemu-nbd.texi +6 -0
index 46fd483..9f9daca 100644
@@ -14,6 +14,12 @@ Export QEMU disk image using NBD protocol.
 @table @option
 @item @var{filename}
  is a disk image filename
+@item --object type,id=@var{id},...props...
+  define a new instance of the @var{type} object class identified by @var{id}.
+  See the @code{qemu(1)} manual page for full details of the properties
+  supported. The common object type that it makes sense to define is the
+  @code{secret} object, which is used to supply passwords and/or encryption
+  keys.
 @item -p, --port=@var{port}
   port to listen on (default @samp{10809})
 @item -o, --offset=@var{offset}
Signature · 1 line
2.5.0
#6 lore raw

Re: [Qemu-devel] [PATCH v4 03/10] qemu-nbd: add support for --object command line arg

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 72 lines

Allow creation of user creatable object types with qemu-nbd via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
      ...other nbd args...
---
 qemu-nbd.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-nbd.texi |  6 ++++++
 2 files changed, 60 insertions(+)
qemu-nbd.c +25 -0
index ede4a54..8e5d36c 100644
@@ -23,9 +23,12 @@
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/opts-visitor.h"
+#include "qom/object_interfaces.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -44,6 +47,7 @@
 #define QEMU_NBD_OPT_AIO           2
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
 static int verbose;
@@ -77,6 +81,9 @@ static void usage(const char *name)
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
+"General purpose options:\n"
+"  --object type,id=ID,...   define an object such as 'secret' for providing\n"
+"                            passwords and/or encryption keys\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -374,6 +381,35 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+

qemu-nbd.c static SocketAddress *nbd_build_socket_address(const char *sockpath, · line 411

Quoted 10 lines
qemu-nbd.c +10 -0
+    user_creatable_add(pdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    QDECREF(pdict);
+
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}

Hm... This looks very similar to the same function in qemu-img.c. And actually, vl.c is a bit different, but not too much either. Wouldn't it make sense to share this code?

Also, now that I compared this to vl.c, there seem to be two differences compared to the tools: Support for filtering (the tools don't seem to need this; passing NULL as the callback could skip it) and an additional obj_unref(obj).

I don't understand much of QOM, so I don't know what the obj_unref() is for, but I'm wondering whether we need to do the same in the tools. Can you explain?

Quoted 20 lines
qemu-nbd.c +2 -0
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -411,6 +447,7 @@ int main(int argc, char **argv)
         { "format", 1, NULL, 'f' },
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
+        { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -428,6 +465,7 @@ int main(int argc, char **argv)
     Error *local_err = NULL;
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
+    QemuOpts *opts;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.

qemu-nbd.c int main(int argc, char **argv) · line 478

Quoted 5 lines
qemu-nbd.c +2 -0
@@ -436,6 +474,8 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);

Like in qemu-img, I'd prefer directly using the pointer.

Kevin

#7 lore raw

Re: [Qemu-devel] [PATCH v4 03/10] qemu-nbd: add support for --object command line arg

Quoted 88 lines · Kevin Wolf

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 72 lines

Allow creation of user creatable object types with qemu-nbd via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
      ...other nbd args...
---
 qemu-nbd.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-nbd.texi |  6 ++++++
 2 files changed, 60 insertions(+)
qemu-nbd.c +25 -0
index ede4a54..8e5d36c 100644
@@ -23,9 +23,12 @@
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/opts-visitor.h"
+#include "qom/object_interfaces.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -44,6 +47,7 @@
 #define QEMU_NBD_OPT_AIO           2
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
 static int verbose;
@@ -77,6 +81,9 @@ static void usage(const char *name)
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
+"General purpose options:\n"
+"  --object type,id=ID,...   define an object such as 'secret' for providing\n"
+"                            passwords and/or encryption keys\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -374,6 +381,35 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+

qemu-nbd.c static SocketAddress *nbd_build_socket_address(const char *sockpath, · line 411

Quoted 10 lines
qemu-nbd.c +10 -0
+    user_creatable_add(pdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    QDECREF(pdict);
+
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}

Hm... This looks very similar to the same function in qemu-img.c. And actually, vl.c is a bit different, but not too much either. Wouldn't it make sense to share this code?

Quoted 4 lines

Also, now that I compared this to vl.c, there seem to be two differences compared to the tools: Support for filtering (the tools don't seem to need this; passing NULL as the callback could skip it) and an additional obj_unref(obj).

Ok, I'll push more of this into qom/object_interfaces.c to allow more sharing.

Quoted 3 lines

I don't understand much of QOM, so I don't know what the obj_unref() is for, but I'm wondering whether we need to do the same in the tools. Can you explain?

Yes, we ought to have the obj_unref too from a technical correctness POV, even though its harmless since we need the object around forever anyway.

Quoted 8 lines

qemu-nbd.c int main(int argc, char **argv) · line 478

Quoted 6 lines
qemu-nbd.c +2 -0
@@ -436,6 +474,8 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);

Like in qemu-img, I'd prefer directly using the pointer.

Yes, will fix

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#8 lore raw

PATCH v4 4/10 qemu-io: add support for --object command line arg

In reply to Daniel P. Berrange

Allow creation of user creatable object types with qemu-io via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-io --object secret,id=sec0,file=mypasswd.txt \
      ...other args...
---
 qemu-io.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
qemu-io.c +54 -0
index d593f19..d1432ea 100644
@@ -18,6 +18,8 @@
 #include "qemu/config-file.h"
 #include "qemu/readline.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/opts-visitor.h"
+#include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
 #include "trace/control.h"
@@ -200,6 +202,8 @@ static void usage(const char *name)
 "Usage: %s [-h] [-V] [-rsnm] [-f FMT] [-c STRING] ... [file]\n"
 "QEMU Disk exerciser\n"
 "\n"
+"  --object OBJECTDEF   define an object such as 'secret' for\n"
+"                       passwords and/or encryption keys\n"
 "  -c, --cmd STRING     execute command with its arguments\n"
 "                       from the given string\n"
 "  -f, --format FMT     specifies the block driver to use\n"
@@ -361,6 +365,38 @@ static void reenable_tty_echo(void)
     qemu_set_tty_echo(STDIN_FILENO, true);
 }
 
+enum {
+    OPTION_OBJECT = 256,
+};
+
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Error *err = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+
+    user_creatable_add(pdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    QDECREF(pdict);
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}
+
 int main(int argc, char **argv)
 {
     int readonly = 0;
@@ -379,6 +415,7 @@ int main(int argc, char **argv)
         { "discard", 1, NULL, 'd' },
         { "cache", 1, NULL, 't' },
         { "trace", 1, NULL, 'T' },
+        { "object", 1, NULL, OPTION_OBJECT },
         { NULL, 0, NULL, 0 }
     };
     int c;
@@ -386,6 +423,7 @@ int main(int argc, char **argv)
     int flags = BDRV_O_UNMAP;
     Error *local_error = NULL;
     QDict *opts = NULL;
+    QemuOpts *qopts = NULL;
 
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
@@ -394,6 +432,8 @@ int main(int argc, char **argv)
     progname = basename(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);
     bdrv_init();
 
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
@@ -445,6 +485,13 @@ int main(int argc, char **argv)
         case 'h':
             usage(progname);
             exit(0);
+        case OPTION_OBJECT:
+            qopts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                            optarg, true);
+            if (!qopts) {
+                exit(1);
+            }
+            break;
         default:
             usage(progname);
             exit(1);
@@ -461,6 +508,13 @@ int main(int argc, char **argv)
         exit(1);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("object"),
+                          object_create,
+                          NULL, &local_error)) {
+        error_report_err(local_error);
+        exit(1);
+    }
+
     /* initialize commands */
     qemuio_add_command(&quit_cmd);
     qemuio_add_command(&open_cmd);
Signature · 1 line
2.5.0
#9 lore raw

PATCH v4 5/10 qemu-io: allow specifying image as a set of options args

In reply to Daniel P. Berrange

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.

---
 qemu-io.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
qemu-io.c +33 -1
index d1432ea..51d8272 100644
@@ -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();
Signature · 1 line
2.5.0
#10 lore raw

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

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 83 lines

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.

---
 qemu-io.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
qemu-io.c +18 -1
index d1432ea..51d8272 100644
@@ -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;

qemu-io.c int main(int argc, char **argv) · line 564

Quoted 10 lines
qemu-io.c +10 -0
+        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.

qemu-io.c int main(int argc, char **argv) · line 572

Quoted 8 lines
qemu-io.c +5 -0
+        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

#11 lore raw

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

Quoted 20 lines · Kevin Wolf

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

qemu-io.c int main(int argc, char **argv) · line 564

Quoted 13 lines
qemu-io.c +12 -1
-    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.

Quoted 8 lines

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

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#12 lore raw

PATCH v4 6/10 qemu-nbd: allow specifying image as a set of options args

In reply to Daniel P. Berrange

Currently qemu-nbd 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-nbd https://127.0.0.1/images/centos7.iso
qemu-nbd /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-nbd --image-opts driver=http,url=https://127.0.0.1/images,sslverify=off
   qemu-nbd --image-opts file=/home/berrange/demo.qcow2

This flag is mutually exclusive with the '-f' flag.

---
 qemu-nbd.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)
qemu-nbd.c +40 -5
index 8e5d36c..764698f 100644
@@ -48,6 +48,7 @@
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
 #define QEMU_NBD_OPT_OBJECT        5
+#define QEMU_NBD_OPT_IMAGE_OPTS    6
 
 static NBDExport *exp;
 static int verbose;
@@ -381,6 +382,16 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+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 */ }
+    },
+};
+
 static QemuOptsList qemu_object_opts = {
     .name = "object",
     .implied_opt_name = "qom-type",
@@ -448,6 +459,7 @@ int main(int argc, char **argv)
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
         { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+        { "image-opts", 0, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -466,6 +478,7 @@ int main(int argc, char **argv)
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     QemuOpts *opts;
+    bool imageOpts = false;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -635,6 +648,9 @@ int main(int argc, char **argv)
                 exit(1);
             }
             break;
+        case QEMU_NBD_OPT_IMAGE_OPTS:
+            imageOpts = true;
+            break;
         case '?':
             error_report("Try `%s --help' for more information.", argv[0]);
             exit(EXIT_FAILURE);
@@ -744,13 +760,32 @@ int main(int argc, char **argv)
     bdrv_init();
     atexit(bdrv_close_all);
 
-    if (fmt) {
-        options = qdict_new();
-        qdict_put(options, "driver", qstring_from_str(fmt));
+    srcpath = argv[optind];
+    if (imageOpts) {
+        char *file = NULL;
+        if (fmt) {
+            error_report("--image-opts and -f are mutually exclusive");
+            exit(EXIT_FAILURE);
+        }
+        opts = qemu_opts_parse_noisily(&file_opts, srcpath, true);
+        if (!opts) {
+            qemu_opts_reset(&file_opts);
+            exit(EXIT_FAILURE);
+        }
+        file = g_strdup(qemu_opt_get(opts, "file"));
+        qemu_opt_unset(opts, "file");
+        options = qemu_opts_to_qdict(opts, NULL);
+        qemu_opts_reset(&file_opts);
+        blk = blk_new_open("hda", file, NULL, options, flags, &local_err);
+        g_free(file);
+    } else {
+        if (fmt) {
+            options = qdict_new();
+            qdict_put(options, "driver", qstring_from_str(fmt));
+        }
+        blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
     }
 
-    srcpath = argv[optind];
-    blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
     if (!blk) {
         error_reportf_err(local_err, "Failed to blk_new_open '%s': ",
                           argv[optind]);
Signature · 1 line
2.5.0
#13 lore raw

PATCH v4 7/10 qemu-img: allow specifying image as a set of options args

In reply to Daniel P. Berrange

Currently qemu-img 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-img info https://127.0.0.1/images/centos7.iso

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-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off

This flag is mutually exclusive with the '-f' / '-F' flags.

---
 qemu-img-cmds.hx |  44 ++++----
 qemu-img.c       | 304 +++++++++++++++++++++++++++++++++++++++++++++++++------
 qemu-img.texi    |   6 ++
 3 files changed, 303 insertions(+), 51 deletions(-)
qemu-img-cmds.hx +22 -22
index 5bb1de7..ee5c770 100644
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [--object objectdef] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [--object objectdef] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object objectdef] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [-f fmt] [-o options] filename [size]")
+    "create [-q] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [--object objectdef] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object objectdef] [--image-opts] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [--object objectdef] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
+    "commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
 STEXI
-@item commit [--object objectdef] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object objectdef] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [--object objectdef] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
+    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
 STEXI
-@item compare [--object objectdef] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object objectdef] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object objectdef] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
-    "info [--object objectdef] [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
 STEXI
-@item info [--object objectdef] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object objectdef] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [--object objectdef] [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] filename")
 STEXI
-@item map [--object objectdef] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+@item map [--object objectdef] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
 ETEXI
 
 DEF("snapshot", img_snapshot,
-    "snapshot [--object objectdef] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
+    "snapshot [--object objectdef] [--image-opts] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
-@item snapshot [--object objectdef] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
+@item snapshot [--object objectdef] [--image-opts] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [--object objectdef] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [--object objectdef] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [--object objectdef] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
-    "resize [--object objectdef] [-q] filename [+ | -]size")
+    "resize [--object objectdef] [--image-opts] [-q] filename [+ | -]size")
 STEXI
-@item resize [--object objectdef] [-q] @var{filename} [+ | -]@var{size}
+@item resize [--object objectdef] [--image-opts] [-q] @var{filename} [+ | -]@var{size}
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [--object objectdef] [-p] [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [--object objectdef] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [--object objectdef] [--image-opts] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
qemu-img.c +275 -29

606 lines not on this page · raw message

qemu-img.texi +6 -0
index da93272..f31e726 100644
@@ -32,6 +32,12 @@ page for a description of the object properties. The only object type that
 it makes sense to define is the @code{secret} object, which is used to
 supply passwords and/or encryption keys.
 
+@item --image-opts
+
+Indicates that the @var{filename} parameter is to be interpreted as a
+full option string, not a plain filename. This parameter is mutually
+exclusive with the @var{-f} and @var{-F} parameters.
+
 @item fmt
 is the disk image format. It is guessed automatically in most cases. See below
 for a description of the supported disk formats.
Signature · 1 line
2.5.0
#14 lore raw

Re: [Qemu-devel] [PATCH v4 07/10] qemu-img: allow specifying image as a set of options args

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 25 lines

Currently qemu-img 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-img info https://127.0.0.1/images/centos7.iso

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-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off

This flag is mutually exclusive with the '-f' / '-F' flags.

qemu-img.c +4 -1
@@ -633,7 +671,20 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (image_opts) {
+        if (fmt) {
+            error_report("--image-opts and --format are mutually exclusive");
+            exit(1);

qemu-img.c static int img_check(int argc, char **argv) · line 687

Quoted 10 lines
qemu-img.c +10 -0
+        }
+        opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+                                       filename, true);
+        if (!opts) {
+            exit(1);
+        }
+        blk = img_open_opts("image", opts, flags);
+    } else {
+        blk = img_open_file("image", filename, fmt, flags, true, quiet);
+    }

This block is duplicated everywhere. Can you make it a function so that it stays a single line in each of the callers?

Kevin

#15 lore raw

Re: [Qemu-devel] [PATCH v4 07/10] qemu-img: allow specifying image as a set of options args

Quoted 31 lines · Kevin Wolf

Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:

Quoted 30 lines

Currently qemu-img 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-img info https://127.0.0.1/images/centos7.iso

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-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off

This flag is mutually exclusive with the '-f' / '-F' flags.

qemu-img.c +9 -1
@@ -633,7 +671,20 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open("image", filename, fmt, flags, true, quiet);
+    if (image_opts) {
+        if (fmt) {
+            error_report("--image-opts and --format are mutually exclusive");
+            exit(1);
+        }
+        opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+                                       filename, true);
+        if (!opts) {
+            exit(1);
Quoted 8 lines

qemu-img.c static int img_check(int argc, char **argv) · line 687

Quoted 5 lines
qemu-img.c +5 -0
+        }
+        blk = img_open_opts("image", opts, flags);
+    } else {
+        blk = img_open_file("image", filename, fmt, flags, true, quiet);
+    }

This block is duplicated everywhere. Can you make it a function so that it stays a single line in each of the callers?

Yes, I've done that and its much nicer now

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#16 lore raw

PATCH v4 8/10 qemu-nbd: don't overlap long option values with short options

In reply to Daniel P. Berrange

When defining values for long options, the normal practice is to start numbering from 256, to avoid overlap with the range of valid values for short options.

---
 qemu-nbd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
qemu-nbd.c +6 -6
index 764698f..bbc79f4 100644
@@ -43,12 +43,12 @@
 #include <pthread.h>
 
 #define SOCKET_PATH                "/var/lock/qemu-nbd-%s"
-#define QEMU_NBD_OPT_CACHE         1
-#define QEMU_NBD_OPT_AIO           2
-#define QEMU_NBD_OPT_DISCARD       3
-#define QEMU_NBD_OPT_DETECT_ZEROES 4
-#define QEMU_NBD_OPT_OBJECT        5
-#define QEMU_NBD_OPT_IMAGE_OPTS    6
+#define QEMU_NBD_OPT_CACHE         256
+#define QEMU_NBD_OPT_AIO           257
+#define QEMU_NBD_OPT_DISCARD       258
+#define QEMU_NBD_OPT_DETECT_ZEROES 259
+#define QEMU_NBD_OPT_OBJECT        260
+#define QEMU_NBD_OPT_IMAGE_OPTS    261
 
 static NBDExport *exp;
 static int verbose;
Signature · 1 line
2.5.0
#17 lore raw

PATCH v4 9/10 qemu-nbd: use no_argument/required_argument constants

In reply to Daniel P. Berrange

When declaring the 'struct option' array, use the standard constants no_argument/required_argument, instead of magic values 0 and 1.

---
 qemu-nbd.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)
qemu-nbd.c +24 -23
index bbc79f4..58e1610 100644
@@ -437,29 +437,30 @@ int main(int argc, char **argv)
     const char *sn_id_or_name = NULL;
     const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
     struct option lopt[] = {
-        { "help", 0, NULL, 'h' },
-        { "version", 0, NULL, 'V' },
-        { "bind", 1, NULL, 'b' },
-        { "port", 1, NULL, 'p' },
-        { "socket", 1, NULL, 'k' },
-        { "offset", 1, NULL, 'o' },
-        { "read-only", 0, NULL, 'r' },
-        { "partition", 1, NULL, 'P' },
-        { "connect", 1, NULL, 'c' },
-        { "disconnect", 0, NULL, 'd' },
-        { "snapshot", 0, NULL, 's' },
-        { "load-snapshot", 1, NULL, 'l' },
-        { "nocache", 0, NULL, 'n' },
-        { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
-        { "aio", 1, NULL, QEMU_NBD_OPT_AIO },
-        { "discard", 1, NULL, QEMU_NBD_OPT_DISCARD },
-        { "detect-zeroes", 1, NULL, QEMU_NBD_OPT_DETECT_ZEROES },
-        { "shared", 1, NULL, 'e' },
-        { "format", 1, NULL, 'f' },
-        { "persistent", 0, NULL, 't' },
-        { "verbose", 0, NULL, 'v' },
-        { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
-        { "image-opts", 0, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
+        { "help", no_argument, NULL, 'h' },
+        { "version", no_argument, NULL, 'V' },
+        { "bind", required_argument, NULL, 'b' },
+        { "port", required_argument, NULL, 'p' },
+        { "socket", required_argument, NULL, 'k' },
+        { "offset", required_argument, NULL, 'o' },
+        { "read-only", no_argument, NULL, 'r' },
+        { "partition", required_argument, NULL, 'P' },
+        { "connect", required_argument, NULL, 'c' },
+        { "disconnect", no_argument, NULL, 'd' },
+        { "snapshot", no_argument, NULL, 's' },
+        { "load-snapshot", required_argument, NULL, 'l' },
+        { "nocache", no_argument, NULL, 'n' },
+        { "cache", required_argument, NULL, QEMU_NBD_OPT_CACHE },
+        { "aio", required_argument, NULL, QEMU_NBD_OPT_AIO },
+        { "discard", required_argument, NULL, QEMU_NBD_OPT_DISCARD },
+        { "detect-zeroes", required_argument, NULL,
+          QEMU_NBD_OPT_DETECT_ZEROES },
+        { "shared", required_argument, NULL, 'e' },
+        { "format", required_argument, NULL, 'f' },
+        { "persistent", no_argument, NULL, 't' },
+        { "verbose", no_argument, NULL, 'v' },
+        { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
+        { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { NULL, 0, NULL, 0 }
     };
     int ch;
Signature · 1 line
2.5.0
#18 lore raw

PATCH v4 10/10 qemu-io: use no_argument/required_argument constants

In reply to Daniel P. Berrange

When declaring the 'struct option' array, use the standard constants no_argument/required_argument, instead of magic values 0 and 1.

---
 qemu-io.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
qemu-io.c +15 -15
index 51d8272..0f759bb 100644
@@ -413,21 +413,21 @@ int main(int argc, char **argv)
     int readonly = 0;
     const char *sopt = "hVc:d:f:rsnmgkt:T:";
     const struct option lopt[] = {
-        { "help", 0, NULL, 'h' },
-        { "version", 0, NULL, 'V' },
-        { "offset", 1, NULL, 'o' },
-        { "cmd", 1, NULL, 'c' },
-        { "format", 1, NULL, 'f' },
-        { "read-only", 0, NULL, 'r' },
-        { "snapshot", 0, NULL, 's' },
-        { "nocache", 0, NULL, 'n' },
-        { "misalign", 0, NULL, 'm' },
-        { "native-aio", 0, NULL, 'k' },
-        { "discard", 1, NULL, 'd' },
-        { "cache", 1, NULL, 't' },
-        { "trace", 1, NULL, 'T' },
-        { "object", 1, NULL, OPTION_OBJECT },
-        { "image-opts", 0, NULL, OPTION_IMAGE_OPTS },
+        { "help", no_argument, NULL, 'h' },
+        { "version", no_argument, NULL, 'V' },
+        { "offset", required_argument, NULL, 'o' },
+        { "cmd", required_argument, NULL, 'c' },
+        { "format", required_argument, NULL, 'f' },
+        { "read-only", no_argument, NULL, 'r' },
+        { "snapshot", no_argument, NULL, 's' },
+        { "nocache", no_argument, NULL, 'n' },
+        { "misalign", no_argument, NULL, 'm' },
+        { "native-aio", no_argument, NULL, 'k' },
+        { "discard", required_argument, NULL, 'd' },
+        { "cache", required_argument, NULL, 't' },
+        { "trace", required_argument, NULL, 'T' },
+        { "object", required_argument, NULL, OPTION_OBJECT },
+        { "image-opts", no_argument, NULL, OPTION_IMAGE_OPTS },
         { NULL, 0, NULL, 0 }
     };
     int c;
Signature · 1 line
2.5.0