* [Qemu-devel] [PATCH 1/7] qom: add user_creatable_add & user_creatable_del methods
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 16:01 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg Daniel P. Berrange
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
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).
We soon need to use this code from qemu-img, qemu-io
and qemu-nbd too, but don't want those to depend on
the monitor.
To avoid this, move object_add to user_creatable_add
an qmp_object_del to user_creatable_del, to the
object_interfaces.c file where they can be easily
shared with all users of QOM
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
hmp.c | 11 ++++--
include/monitor/monitor.h | 3 --
include/qom/object_interfaces.h | 31 +++++++++++++++++
qmp.c | 75 ++++------------------------------------
qom/object_interfaces.c | 76 +++++++++++++++++++++++++++++++++++++++++
vl.c | 8 +++--
6 files changed, 127 insertions(+), 77 deletions(-)
diff --git a/hmp.c b/hmp.c
index c2b2c16..57fa3f5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -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"
@@ -1670,6 +1671,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
void *dummy = NULL;
OptsVisitor *ov;
QDict *pdict;
+ Object *obj = NULL;
opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
if (err) {
@@ -1696,12 +1698,12 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
goto out_end;
}
- object_add(type, id, pdict, opts_get_visitor(ov), &err);
+ obj = user_creatable_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);
+ user_creatable_del(id, NULL);
}
error_propagate(&err, err_end);
out_clean:
@@ -1712,6 +1714,9 @@ out_clean:
g_free(id);
g_free(type);
g_free(dummy);
+ if (obj) {
+ object_unref(obj);
+ }
out:
hmp_handle_error(mon, &err);
@@ -1944,7 +1949,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);
}
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 91b95ae..aa0f373 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -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);
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 283ae0d..3e2afeb 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,8 @@
#define OBJECT_INTERFACES_H
#include "qom/object.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/visitor.h"
#define TYPE_USER_CREATABLE "user-creatable"
@@ -72,4 +74,33 @@ void user_creatable_complete(Object *obj, Error **errp);
* from implements USER_CREATABLE interface.
*/
bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
+
+/**
+ * user_creatable_add:
+ * @type: the object type name
+ * @id: the unique ID for the object
+ * @qdict: the object parameters
+ * @v: the visitor
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object @type, placing
+ * it in the object composition tree with name @id, initializing
+ * it with properties from @qdict
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add(const char *type, const char *id,
+ const QDict *qdict,
+ Visitor *v, Error **errp);
+
+/**
+ * user_creatable_del:
+ * @id: the unique ID for the object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Delete an instance of the user creatable object identified
+ * by @id.
+ */
+void user_creatable_del(const char *id, Error **errp);
+
#endif
diff --git a/qmp.c b/qmp.c
index 0a1fa19..70fdecd 100644
--- a/qmp.c
+++ b/qmp.c
@@ -622,65 +622,13 @@ void qmp_add_client(const char *protocol, const char *fdname,
close(fd);
}
-void object_add(const char *type, const char *id, const QDict *qdict,
- Visitor *v, Error **errp)
-{
- Object *obj;
- ObjectClass *klass;
- const QDictEntry *e;
- Error *local_err = NULL;
-
- klass = object_class_by_name(type);
- if (!klass) {
- error_setg(errp, "invalid object type: %s", type);
- return;
- }
-
- if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
- error_setg(errp, "object type '%s' isn't supported by object-add",
- type);
- return;
- }
-
- if (object_class_is_abstract(klass)) {
- error_setg(errp, "object type '%s' is abstract", type);
- return;
- }
-
- obj = object_new(type);
- if (qdict) {
- for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
- object_property_set(obj, v, e->key, &local_err);
- if (local_err) {
- goto out;
- }
- }
- }
-
- object_property_add_child(object_get_objects_root(),
- id, obj, &local_err);
- if (local_err) {
- goto out;
- }
-
- user_creatable_complete(obj, &local_err);
- if (local_err) {
- object_property_del(object_get_objects_root(),
- id, &error_abort);
- goto out;
- }
-out:
- if (local_err) {
- error_propagate(errp, local_err);
- }
- object_unref(obj);
-}
void qmp_object_add(const char *type, const char *id,
bool has_props, QObject *props, Error **errp)
{
const QDict *pdict = NULL;
QmpInputVisitor *qiv;
+ Object *obj;
if (props) {
pdict = qobject_to_qdict(props);
@@ -691,27 +639,16 @@ void qmp_object_add(const char *type, const char *id,
}
qiv = qmp_input_visitor_new(props);
- object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp);
+ obj = user_creatable_add(type, id, pdict, qmp_input_get_visitor(qiv), errp);
qmp_input_visitor_cleanup(qiv);
+ if (obj) {
+ object_unref(obj);
+ }
}
void qmp_object_del(const char *id, Error **errp)
{
- Object *container;
- Object *obj;
-
- container = object_get_objects_root();
- obj = object_resolve_path_component(container, id);
- if (!obj) {
- error_setg(errp, "object id not found");
- return;
- }
-
- if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
- error_setg(errp, "%s is in use, can not be deleted", id);
- return;
- }
- object_unparent(obj);
+ user_creatable_del(id, errp);
}
MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index a66cd60..d94995f 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -30,6 +30,82 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
}
}
+Object *user_creatable_add(const char *type, const char *id,
+ const QDict *qdict,
+ Visitor *v, Error **errp)
+{
+ Object *obj;
+ ObjectClass *klass;
+ const QDictEntry *e;
+ Error *local_err = NULL;
+
+ klass = object_class_by_name(type);
+ if (!klass) {
+ error_setg(errp, "invalid object type: %s", type);
+ return NULL;
+ }
+
+ if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
+ error_setg(errp, "object type '%s' isn't supported by object-add",
+ type);
+ return NULL;
+ }
+
+ if (object_class_is_abstract(klass)) {
+ error_setg(errp, "object type '%s' is abstract", type);
+ return NULL;
+ }
+
+ obj = object_new(type);
+ if (qdict) {
+ for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+ object_property_set(obj, v, e->key, &local_err);
+ if (local_err) {
+ goto out;
+ }
+ }
+ }
+
+ object_property_add_child(object_get_objects_root(),
+ id, obj, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
+ user_creatable_complete(obj, &local_err);
+ if (local_err) {
+ object_property_del(object_get_objects_root(),
+ id, &error_abort);
+ goto out;
+ }
+out:
+ if (local_err) {
+ error_propagate(errp, local_err);
+ object_unref(obj);
+ return NULL;
+ }
+ return obj;
+}
+
+void user_creatable_del(const char *id, Error **errp)
+{
+ Object *container;
+ Object *obj;
+
+ container = object_get_objects_root();
+ obj = object_resolve_path_component(container, id);
+ if (!obj) {
+ error_setg(errp, "object id not found");
+ return;
+ }
+
+ if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
+ error_setg(errp, "%s is in use, can not be deleted", id);
+ return;
+ }
+ object_unparent(obj);
+}
+
static void register_types(void)
{
static const TypeInfo uc_interface_info = {
diff --git a/vl.c b/vl.c
index 5aaea77..812e039 100644
--- a/vl.c
+++ b/vl.c
@@ -2834,6 +2834,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
OptsVisitor *ov;
QDict *pdict;
bool (*type_predicate)(const char *) = opaque;
+ Object *obj = NULL;
ov = opts_visitor_new(opts);
pdict = qemu_opts_to_qdict(opts, NULL);
@@ -2858,13 +2859,13 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
goto out;
}
- object_add(type, id, pdict, opts_get_visitor(ov), &err);
+ obj = user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
if (err) {
goto out;
}
visit_end_struct(opts_get_visitor(ov), &err);
if (err) {
- qmp_object_del(id, NULL);
+ user_creatable_del(id, NULL);
}
out:
@@ -2874,6 +2875,9 @@ out:
g_free(id);
g_free(type);
g_free(dummy);
+ if (obj) {
+ object_unref(obj);
+ }
if (err) {
error_report_err(err);
return -1;
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] qom: add user_creatable_add & user_creatable_del methods
2015-12-22 11:06 ` [Qemu-devel] [PATCH 1/7] qom: add user_creatable_add & user_creatable_del methods Daniel P. Berrange
@ 2015-12-22 16:01 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2015-12-22 16:01 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> 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).
>
> We soon need to use this code from qemu-img, qemu-io
> and qemu-nbd too, but don't want those to depend on
> the monitor.
>
> To avoid this, move object_add to user_creatable_add
> an qmp_object_del to user_creatable_del, to the
> object_interfaces.c file where they can be easily
> shared with all users of QOM
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> hmp.c | 11 ++++--
> include/monitor/monitor.h | 3 --
> include/qom/object_interfaces.h | 31 +++++++++++++++++
> qmp.c | 75 ++++------------------------------------
> qom/object_interfaces.c | 76 +++++++++++++++++++++++++++++++++++++++++
> vl.c | 8 +++--
> 6 files changed, 127 insertions(+), 77 deletions(-)
Looks like fairly straightforward code motion.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
2015-12-22 11:06 ` [Qemu-devel] [PATCH 1/7] qom: add user_creatable_add & user_creatable_del methods Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 16:24 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: " Daniel P. Berrange
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Allow creation of user creatable object types with qemu-img
via a --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.
# echo -n letmein > mypasswd.txt
# qemu-img info --object secret,id=sec0,file=mypasswd.txt \
...other info args...
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-img-cmds.hx | 44 ++++----
qemu-img.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
qemu-img.texi | 8 ++
3 files changed, 322 insertions(+), 30 deletions(-)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 9567774..5bb1de7 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -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
diff --git a/qemu-img.c b/qemu-img.c
index 3d48b4f..47f0006 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -23,12 +23,15 @@
*/
#include "qapi-visit.h"
#include "qapi/qmp-output-visitor.h"
+#include "qapi/opts-visitor.h"
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qjson.h"
#include "qemu-common.h"
+#include "qemu/config-file.h"
#include "qemu/option.h"
#include "qemu/error-report.h"
#include "qemu/osdep.h"
+#include "qom/object_interfaces.h"
#include "sysemu/sysemu.h"
#include "sysemu/block-backend.h"
#include "block/block_int.h"
@@ -47,6 +50,7 @@ typedef struct img_cmd_t {
enum {
OPTION_OUTPUT = 256,
OPTION_BACKING_CHAIN = 257,
+ OPTION_OBJECT = 258,
};
typedef enum OutputFormat {
@@ -94,6 +98,11 @@ 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 \n"
+ " the @code{qemu(1)} manual page for a description of the object\n"
+ " properties. The only object type that it makes sense to define\n"
+ " is the @code{secret} object, which is used to supply passwords\n"
+ " 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"
@@ -154,6 +163,67 @@ 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;
+ char *type = NULL;
+ char *id = NULL;
+ void *dummy = NULL;
+ OptsVisitor *ov;
+ QDict *pdict;
+
+ ov = opts_visitor_new(opts);
+ pdict = qemu_opts_to_qdict(opts, NULL);
+
+ visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "qom-type");
+ visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "id");
+ visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+ if (err) {
+ goto out;
+ }
+
+ user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
+ if (err) {
+ goto out;
+ }
+ visit_end_struct(opts_get_visitor(ov), &err);
+ if (err) {
+ user_creatable_del(id, NULL);
+ }
+
+out:
+ opts_visitor_cleanup(ov);
+
+ QDECREF(pdict);
+ g_free(id);
+ g_free(type);
+ g_free(dummy);
+ if (err) {
+ error_report_err(err);
+ return -1;
+ }
+ return 0;
+}
+
static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
{
int ret = 0;
@@ -275,9 +345,17 @@ static int img_create(int argc, char **argv)
char *options = NULL;
Error *local_err = NULL;
bool quiet = false;
+ QemuOpts *opts;
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;
}
@@ -319,6 +397,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);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -334,6 +419,12 @@ static int img_create(int argc, char **argv)
}
optind++;
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* Get image size, if specified */
if (optind < argc) {
int64_t sval;
@@ -492,6 +583,7 @@ static int img_check(int argc, char **argv)
int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
ImageCheck *check;
bool quiet = false;
+ QemuOpts *opts;
fmt = NULL;
output = NULL;
@@ -503,6 +595,7 @@ static int img_check(int argc, char **argv)
{"format", required_argument, 0, 'f'},
{"repair", required_argument, 0, 'r'},
{"output", required_argument, 0, OPTION_OUTPUT},
+ {"object", required_argument, 0, OPTION_OBJECT},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "hf:r:T:q",
@@ -539,6 +632,13 @@ static int img_check(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
if (optind != argc - 1) {
@@ -555,6 +655,12 @@ static int img_check(int argc, char **argv)
return 1;
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
ret = bdrv_parse_cache_flags(cache, &flags);
if (ret < 0) {
error_report("Invalid source cache option: %s", cache);
@@ -673,12 +779,20 @@ static int img_commit(int argc, char **argv)
bool progress = false, quiet = false, drop = false;
Error *local_err = NULL;
CommonBlockJobCBInfo cbi;
+ QemuOpts *opts;
fmt = NULL;
cache = BDRV_DEFAULT_CACHE;
base = NULL;
for(;;) {
- c = getopt(argc, argv, "f:ht:b:dpq");
+ 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:ht:b:dpq",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -707,6 +821,13 @@ static int img_commit(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -720,6 +841,12 @@ static int img_commit(int argc, char **argv)
}
filename = argv[optind++];
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
flags = BDRV_O_RDWR | BDRV_O_UNMAP;
ret = bdrv_parse_cache_flags(cache, &flags);
if (ret < 0) {
@@ -976,10 +1103,18 @@ static int img_compare(int argc, char **argv)
int64_t nb_sectors;
int c, pnum;
uint64_t progress_base;
+ QemuOpts *opts;
cache = BDRV_DEFAULT_CACHE;
for (;;) {
- c = getopt(argc, argv, "hf:F:T:pqs");
+ 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, "hf:F:T:pqs",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -1006,6 +1141,13 @@ static int img_compare(int argc, char **argv)
case 's':
strict = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -1021,6 +1163,12 @@ static int img_compare(int argc, char **argv)
filename1 = argv[optind++];
filename2 = argv[optind++];
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* Initialize before goto out */
qemu_progress_init(progress, 2.0);
@@ -1540,7 +1688,14 @@ static int img_convert(int argc, char **argv)
compress = 0;
skip_create = 0;
for(;;) {
- c = getopt(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn");
+ 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, "hf:O:B:ce6o:s:l:S:pt:T:qn",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -1631,9 +1786,22 @@ static int img_convert(int argc, char **argv)
case 'n':
skip_create = 1;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* Initialize before goto out */
if (quiet) {
progress = 0;
@@ -2066,6 +2234,7 @@ static int img_info(int argc, char **argv)
bool chain = false;
const char *filename, *fmt, *output;
ImageInfoList *list;
+ QemuOpts *opts;
fmt = NULL;
output = NULL;
@@ -2076,6 +2245,7 @@ static int img_info(int argc, char **argv)
{"format", required_argument, 0, 'f'},
{"output", required_argument, 0, OPTION_OUTPUT},
{"backing-chain", no_argument, 0, OPTION_BACKING_CHAIN},
+ {"object", required_argument, 0, OPTION_OBJECT},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:h",
@@ -2097,6 +2267,13 @@ static int img_info(int argc, char **argv)
case OPTION_BACKING_CHAIN:
chain = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
if (optind != argc - 1) {
@@ -2113,6 +2290,12 @@ static int img_info(int argc, char **argv)
return 1;
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
list = collect_image_info_list(filename, fmt, chain);
if (!list) {
return 1;
@@ -2236,6 +2419,7 @@ static int img_map(int argc, char **argv)
int64_t length;
MapEntry curr = { .length = 0 }, next;
int ret = 0;
+ QemuOpts *opts;
fmt = NULL;
output = NULL;
@@ -2245,6 +2429,7 @@ static int img_map(int argc, char **argv)
{"help", no_argument, 0, 'h'},
{"format", required_argument, 0, 'f'},
{"output", required_argument, 0, OPTION_OUTPUT},
+ {"object", required_argument, 0, OPTION_OBJECT},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:h",
@@ -2263,6 +2448,13 @@ static int img_map(int argc, char **argv)
case OPTION_OUTPUT:
output = optarg;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
if (optind != argc - 1) {
@@ -2279,6 +2471,12 @@ static int img_map(int argc, char **argv)
return 1;
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
blk = img_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
if (!blk) {
return 1;
@@ -2344,11 +2542,19 @@ static int img_snapshot(int argc, char **argv)
qemu_timeval tv;
bool quiet = false;
Error *err = NULL;
+ QemuOpts *opts;
bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
/* Parse commandline parameters */
for(;;) {
- c = getopt(argc, argv, "la:c:d:hq");
+ 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, "la:c:d:hq",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -2392,6 +2598,13 @@ static int img_snapshot(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -2400,6 +2613,12 @@ static int img_snapshot(int argc, char **argv)
}
filename = argv[optind++];
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* Open the image */
blk = img_open("image", filename, NULL, bdrv_oflags, true, quiet);
if (!blk) {
@@ -2466,6 +2685,7 @@ static int img_rebase(int argc, char **argv)
int progress = 0;
bool quiet = false;
Error *local_err = NULL;
+ QemuOpts *opts;
/* Parse commandline parameters */
fmt = NULL;
@@ -2474,7 +2694,14 @@ static int img_rebase(int argc, char **argv)
out_baseimg = NULL;
out_basefmt = NULL;
for(;;) {
- c = getopt(argc, argv, "hf:F:b:upt:T: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, "hf:F:b:upt:T:q",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -2507,6 +2734,13 @@ static int img_rebase(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -2522,6 +2756,12 @@ static int img_rebase(int argc, char **argv)
}
filename = argv[optind++];
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
qemu_progress_init(progress, 2.0);
qemu_progress_print(0, 100);
@@ -2782,6 +3022,7 @@ static int img_resize(int argc, char **argv)
bool quiet = false;
BlockBackend *blk = NULL;
QemuOpts *param;
+ QemuOpts *opts;
static QemuOptsList resize_options = {
.name = "resize_options",
.head = QTAILQ_HEAD_INITIALIZER(resize_options.head),
@@ -2808,7 +3049,14 @@ static int img_resize(int argc, char **argv)
/* Parse getopt arguments */
fmt = NULL;
for(;;) {
- c = getopt(argc, argv, "f:hq");
+ 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:hq",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -2823,6 +3071,13 @@ static int img_resize(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
if (optind != argc - 1) {
@@ -2830,6 +3085,12 @@ static int img_resize(int argc, char **argv)
}
filename = argv[optind++];
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* Choose grow, shrink, or absolute resize mode */
switch (size[0]) {
case '+':
@@ -2920,7 +3181,14 @@ static int img_amend(int argc, char **argv)
cache = BDRV_DEFAULT_CACHE;
for (;;) {
- c = getopt(argc, argv, "ho:f:t:pq");
+ 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, "ho:f:t:pq",
+ long_options, &option_index);
if (c == -1) {
break;
}
@@ -2956,6 +3224,13 @@ static int img_amend(int argc, char **argv)
case 'q':
quiet = true;
break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -2963,6 +3238,12 @@ static int img_amend(int argc, char **argv)
error_exit("Must specify options (-o)");
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
if (quiet) {
progress = false;
}
@@ -3085,6 +3366,9 @@ int main(int argc, char **argv)
}
cmdname = argv[1];
+ module_call_init(MODULE_INIT_QOM);
+ qemu_add_opts(&qemu_object_opts);
+
/* find the command */
for (cmd = img_cmds; cmd->name != NULL; cmd++) {
if (!strcmp(cmdname, cmd->name)) {
diff --git a/qemu-img.texi b/qemu-img.texi
index 55c6be3..da93272 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -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.
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg
2015-12-22 11:06 ` [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg Daniel P. Berrange
@ 2015-12-22 16:24 ` Eric Blake
2015-12-22 17:21 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 16:24 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 4795 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Allow creation of user creatable object types with qemu-img
> via a --object command line arg. This will be used to supply
Does this read better as "a dash-dash-object", or "an object", in which
case you may have an article mismatch? You can skirt the issue by
adding an adjective: "a new --object" works with either pronunciation of
"--object" :)
> passwords and/or encryption keys to the various block driver
> backends via the recently added 'secret' object type.
>
> # echo -n letmein > mypasswd.txt
'echo -n' is not portable; although it doesn't matter here, I tend to
favor 'printf letmein' for both its portability and for less typing.
> # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
> ...other info args...
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qemu-img-cmds.hx | 44 ++++----
> qemu-img.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> qemu-img.texi | 8 ++
> 3 files changed, 322 insertions(+), 30 deletions(-)
How will libvirt discover whether qemu-img is new enough to support this
syntax? Then again, qemu-img isn't used quite as heavily as qemu, and
the speedups we gain by using QMP instead of -help scraping on qemu
don't matter quite as much as what we can attempt with -help scraping on
qemu-img.
> @@ -94,6 +98,11 @@ 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 \n"
Trailing whitespace on the user's terminal.
> + " the @code{qemu(1)} manual page for a description of the object\n"
> + " properties. The only object type that it makes sense to define\n"
> + " is the @code{secret} object, which is used to supply passwords\n"
> + " 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"
You wrapped the text to fit in 80 source columns, but the lines below
wrapped to keep it at 80 user display columns (at the expense of longer
source text). I'd actually lean towards the longer lines in this case,
even if we have to manually ignore checkpatch.pl.
[GNU coreutils does it like:
printf("\
long line starting in column 0\n\
etc.");
so that you can fit much closer to 80 output characters while still
staying within 80 source columns; but I don't think we need the churn of
taking on that style]
> +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +{
> + Error *err = NULL;
> + char *type = NULL;
> + char *id = NULL;
> + void *dummy = NULL;
Drop this.
> + OptsVisitor *ov;
> + QDict *pdict;
Add a Visitor *v; helper variable.
> +
> + ov = opts_visitor_new(opts);
> + pdict = qemu_opts_to_qdict(opts, NULL);
> +
> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
This conflicts with my pending patches:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03863.html
If mine go in first, you'll want this to be:
visit_start_struct(v, NULL, NULL, 0, &err);
And even if yours goes in first, you should make it look more like this,
so I don't have to fix it up after you:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03862.html
(since it looks like you copied from there anyways :)
> +
> + user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
> + if (err) {
> + goto out;
> + }
> + visit_end_struct(opts_get_visitor(ov), &err);
visit_end_struct() needs to be called unconditionally if
visit_start_struct() succeeded. Again, if my series goes in first,
rebase it to look like my changes to vl.c; if yours goes in first, I'll
have to touch up your additions to match what I do elsewhere in my series.
> @@ -319,6 +397,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);
> + if (!opts) {
> + exit(1);
Not for this patch, but maybe someday we should switch to
exit(EXIT_FAILURE) throughout the file.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg
2015-12-22 16:24 ` Eric Blake
@ 2015-12-22 17:21 ` Daniel P. Berrange
0 siblings, 0 replies; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 17:21 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 09:24:00AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Allow creation of user creatable object types with qemu-img
> > via a --object command line arg. This will be used to supply
>
> Does this read better as "a dash-dash-object", or "an object", in which
> case you may have an article mismatch? You can skirt the issue by
> adding an adjective: "a new --object" works with either pronunciation of
> "--object" :)
Heh, ok
> > passwords and/or encryption keys to the various block driver
> > backends via the recently added 'secret' object type.
> >
> > # echo -n letmein > mypasswd.txt
>
> 'echo -n' is not portable; although it doesn't matter here, I tend to
> favor 'printf letmein' for both its portability and for less typing.
Yep, I fixed my other patches to use printf previously too.
> > qemu-img-cmds.hx | 44 ++++----
> > qemu-img.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > qemu-img.texi | 8 ++
> > 3 files changed, 322 insertions(+), 30 deletions(-)
>
> How will libvirt discover whether qemu-img is new enough to support this
> syntax? Then again, qemu-img isn't used quite as heavily as qemu, and
> the speedups we gain by using QMP instead of -help scraping on qemu
> don't matter quite as much as what we can attempt with -help scraping on
> qemu-img.
Yeah, qemu-img feature detection is an outstanding unsolved problem
that I don't really have an answer for. In general though, I think
libvirt will just take the approach of blindly try to use it, if and
only if, we actually need the new feature. That should not create
us any back-compat problems, and get us moderately acceptable error
reporting if we try to use new feature with old qemu-img.
> > + " the @code{qemu(1)} manual page for a description of the object\n"
> > + " properties. The only object type that it makes sense to define\n"
> > + " is the @code{secret} object, which is used to supply passwords\n"
> > + " 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"
>
> You wrapped the text to fit in 80 source columns, but the lines below
> wrapped to keep it at 80 user display columns (at the expense of longer
> source text). I'd actually lean towards the longer lines in this case,
> even if we have to manually ignore checkpatch.pl.
Yep, good point, I missed that distinction.
> [GNU coreutils does it like:
>
> printf("\
> long line starting in column 0\n\
> etc.");
>
> so that you can fit much closer to 80 output characters while still
> staying within 80 source columns; but I don't think we need the churn of
> taking on that style]
>
>
> > +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > + Error *err = NULL;
> > + char *type = NULL;
> > + char *id = NULL;
> > + void *dummy = NULL;
>
> Drop this.
>
> > + OptsVisitor *ov;
> > + QDict *pdict;
>
> Add a Visitor *v; helper variable.
>
> > +
> > + ov = opts_visitor_new(opts);
> > + pdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
>
> This conflicts with my pending patches:
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03863.html
>
> If mine go in first, you'll want this to be:
>
> visit_start_struct(v, NULL, NULL, 0, &err);
>
> And even if yours goes in first, you should make it look more like this,
> so I don't have to fix it up after you:
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03862.html
>
> (since it looks like you copied from there anyways :)
Yep, ok.
>
>
> > +
> > + user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
> > + if (err) {
> > + goto out;
> > + }
> > + visit_end_struct(opts_get_visitor(ov), &err);
>
> visit_end_struct() needs to be called unconditionally if
> visit_start_struct() succeeded. Again, if my series goes in first,
> rebase it to look like my changes to vl.c; if yours goes in first, I'll
> have to touch up your additions to match what I do elsewhere in my series.
>
> > @@ -319,6 +397,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);
> > + if (!opts) {
> > + exit(1);
>
> Not for this patch, but maybe someday we should switch to
> exit(EXIT_FAILURE) throughout the file.
Yeah, that came to mind when I was working on these patches. A cleanup
for another day though, lest my number of pending patches exceed 100 !
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 3/7] qemu-nbd: add support for --object command line arg
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
2015-12-22 11:06 ` [Qemu-devel] [PATCH 1/7] qom: add user_creatable_add & user_creatable_del methods Daniel P. Berrange
2015-12-22 11:06 ` [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 16:49 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 4/7] qemu-io: " Daniel P. Berrange
` (3 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Allow creation of user creatable object types with qemu-nbd
via a --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.
# echo -n letmein > mypasswd.txt
# qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
...other nbd args...
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-nbd.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-nbd.texi | 7 +++++
2 files changed, 92 insertions(+)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 65dc30c..b4b6681 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -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>
@@ -45,6 +48,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;
@@ -78,6 +82,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"
@@ -380,6 +387,67 @@ 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;
+ char *type = NULL;
+ char *id = NULL;
+ void *dummy = NULL;
+ OptsVisitor *ov;
+ QDict *pdict;
+
+ ov = opts_visitor_new(opts);
+ pdict = qemu_opts_to_qdict(opts, NULL);
+
+ visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "qom-type");
+ visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "id");
+ visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+ if (err) {
+ goto out;
+ }
+
+ user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
+ if (err) {
+ goto out;
+ }
+ visit_end_struct(opts_get_visitor(ov), &err);
+ if (err) {
+ user_creatable_del(id, NULL);
+ }
+
+out:
+ opts_visitor_cleanup(ov);
+
+ QDECREF(pdict);
+ g_free(id);
+ g_free(type);
+ g_free(dummy);
+ if (err) {
+ error_report_err(err);
+ return -1;
+ }
+ return 0;
+}
+
int main(int argc, char **argv)
{
BlockBackend *blk;
@@ -417,6 +485,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;
@@ -434,6 +503,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.
@@ -442,6 +512,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) {
@@ -578,6 +650,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 '?':
errx(EXIT_FAILURE, "Try `%s --help' for more information.",
argv[0]);
@@ -590,6 +669,12 @@ int main(int argc, char **argv)
argv[0]);
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
if (disconnect) {
fd = open(argv[optind], O_RDWR);
if (fd < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 46fd483..3b2004e 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -14,6 +14,13 @@ 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 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 -p, --port=@var{port}
port to listen on (default @samp{10809})
@item -o, --offset=@var{offset}
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] qemu-nbd: add support for --object command line arg
2015-12-22 11:06 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: " Daniel P. Berrange
@ 2015-12-22 16:49 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2015-12-22 16:49 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Allow creation of user creatable object types with qemu-nbd
> via a --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.
>
> # echo -n letmein > mypasswd.txt
> # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
> ...other nbd args...
Same comments as on 2/7.
> @@ -45,6 +48,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
Pre-existing, but these are unsafe; they conflict with actual byte
values. As long as you are touching this, you should fix them to start
at 256 (a separate patch wouldn't hurt).
> +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +{
> + Error *err = NULL;
> + char *type = NULL;
> + char *id = NULL;
> + void *dummy = NULL;
> + OptsVisitor *ov;
> + QDict *pdict;
> +
> + ov = opts_visitor_new(opts);
> + pdict = qemu_opts_to_qdict(opts, NULL);
> +
> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
Same comments as on 2/7.
> @@ -417,6 +485,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 }
Pre-existing, but 0 and 1 are magic numbers; I prefer the symbolic names
no_argument and required_argument (and optional_argument for 2).
> +++ b/qemu-nbd.texi
> @@ -14,6 +14,13 @@ 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 only object type that it makes sense to define
> + is the @code{secret} object, which is used to supply
> + passwords and/or encryption keys.
Awfully short line-wrapping; although it doesn't matter in the final
generated docs.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
` (2 preceding siblings ...)
2015-12-22 11:06 ` [Qemu-devel] [PATCH 3/7] qemu-nbd: " Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 16:55 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args Daniel P. Berrange
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Allow creation of user creatable object types with qemu-io
via a --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.
# echo -n letmein > mypasswd.txt
# qemu-io --object secret,id=sec0,file=mypasswd.txt \
...other args...
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-io.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)
diff --git a/qemu-io.c b/qemu-io.c
index 269f17c..cf1dac6 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -21,6 +21,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"
@@ -205,6 +207,9 @@ 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 a object such as 'secret' for\n"
+" providing passwords and/or encryption\n"
+" 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"
@@ -366,6 +371,71 @@ static void reenable_tty_echo(void)
qemu_set_tty_echo(STDIN_FILENO, true);
}
+enum {
+ OPTION_OBJECT = 258,
+};
+
+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;
+ char *type = NULL;
+ char *id = NULL;
+ void *dummy = NULL;
+ OptsVisitor *ov;
+ QDict *pdict;
+
+ ov = opts_visitor_new(opts);
+ pdict = qemu_opts_to_qdict(opts, NULL);
+
+ visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "qom-type");
+ visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+ if (err) {
+ goto out;
+ }
+
+ qdict_del(pdict, "id");
+ visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+ if (err) {
+ goto out;
+ }
+
+ user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
+ if (err) {
+ goto out;
+ }
+ visit_end_struct(opts_get_visitor(ov), &err);
+ if (err) {
+ user_creatable_del(id, NULL);
+ }
+
+out:
+ opts_visitor_cleanup(ov);
+
+ QDECREF(pdict);
+ g_free(id);
+ g_free(type);
+ g_free(dummy);
+ if (err) {
+ error_report_err(err);
+ return -1;
+ }
+ return 0;
+}
+
int main(int argc, char **argv)
{
int readonly = 0;
@@ -384,6 +454,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;
@@ -391,6 +462,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);
@@ -399,6 +471,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) {
@@ -450,6 +524,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);
@@ -466,6 +547,12 @@ int main(int argc, char **argv)
exit(1);
}
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ NULL, NULL)) {
+ exit(1);
+ }
+
/* initialize commands */
qemuio_add_command(&quit_cmd);
qemuio_add_command(&open_cmd);
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg
2015-12-22 11:06 ` [Qemu-devel] [PATCH 4/7] qemu-io: " Daniel P. Berrange
@ 2015-12-22 16:55 ` Eric Blake
2015-12-22 17:24 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 16:55 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Allow creation of user creatable object types with qemu-io
> via a --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.
>
> # echo -n letmein > mypasswd.txt
> # qemu-io --object secret,id=sec0,file=mypasswd.txt \
> ...other args...
Same comments as on 3/7.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qemu-io.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 87 insertions(+)
> @@ -205,6 +207,9 @@ 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 a object such as 'secret' for\n"
s/a object/an object/
> +" providing passwords and/or encryption\n"
> +" keys\n"
3 lines feels long; you got away with only 2 lines in 3/7 by using
longer line wrapping, while still fitting in the user's 80 column output:
+" --object type,id=ID,... define an object such as 'secret' for
providing\n"
+" passwords and/or encryption keys\n"
>
> +enum {
> + OPTION_OBJECT = 258,
> +};
256 would work. But 258 doesn't hurt.
> +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +{
> + Error *err = NULL;
> + char *type = NULL;
> + char *id = NULL;
> + void *dummy = NULL;
> + OptsVisitor *ov;
> + QDict *pdict;
> +
> + ov = opts_visitor_new(opts);
> + pdict = qemu_opts_to_qdict(opts, NULL);
> +
> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
Same comments as on 3/7.
We now have 5 very similar functions (hmp.c, vl.c, and your three
additions); should this be factored into a common reusable function
rather than open-coding it into each client?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg
2015-12-22 16:55 ` Eric Blake
@ 2015-12-22 17:24 ` Daniel P. Berrange
2015-12-23 18:02 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 17:24 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 09:55:17AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Allow creation of user creatable object types with qemu-io
> > via a --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.
> >
> > # echo -n letmein > mypasswd.txt
> > # qemu-io --object secret,id=sec0,file=mypasswd.txt \
> > ...other args...
>
> Same comments as on 3/7.
>
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > qemu-io.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 87 insertions(+)
>
> > @@ -205,6 +207,9 @@ 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 a object such as 'secret' for\n"
>
> s/a object/an object/
>
> > +" providing passwords and/or encryption\n"
> > +" keys\n"
>
> 3 lines feels long; you got away with only 2 lines in 3/7 by using
> longer line wrapping, while still fitting in the user's 80 column output:
>
> +" --object type,id=ID,... define an object such as 'secret' for
> providing\n"
> +" passwords and/or encryption keys\n"
>
>
> >
> > +enum {
> > + OPTION_OBJECT = 258,
> > +};
>
> 256 would work. But 258 doesn't hurt.
>
>
> > +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > + Error *err = NULL;
> > + char *type = NULL;
> > + char *id = NULL;
> > + void *dummy = NULL;
> > + OptsVisitor *ov;
> > + QDict *pdict;
> > +
> > + ov = opts_visitor_new(opts);
> > + pdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
>
> Same comments as on 3/7.
>
> We now have 5 very similar functions (hmp.c, vl.c, and your three
> additions); should this be factored into a common reusable function
> rather than open-coding it into each client?
Yeah, it is a bit questionable to have this duplication. I already saved
a fair bit with the first patch introducing the shared user_createable_new.
I was a little reluctant to move this 'object_create' method into the
qom/ code though, since I hate the idea of the legacy 'QemuOpts' data
anywhere near those nice new APIs. I guess I could perhaps just keep the
qemu_opts_to_qdict() call in the caller.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg
2015-12-22 17:24 ` Daniel P. Berrange
@ 2015-12-23 18:02 ` Paolo Bonzini
2015-12-23 19:25 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2015-12-23 18:02 UTC (permalink / raw)
To: Daniel P. Berrange, Eric Blake
Cc: Kevin Wolf, Andreas Färber, qemu-devel, qemu-block,
Markus Armbruster
On 22/12/2015 18:24, Daniel P. Berrange wrote:
> I was a little reluctant to move this 'object_create' method into the
> qom/ code though, since I hate the idea of the legacy 'QemuOpts' data
> anywhere near those nice new APIs. I guess I could perhaps just keep the
> qemu_opts_to_qdict() call in the caller.
I disagree with calling QemuOpts legacy, but I agree that this shouldn't
be in qom/object_interfaces.c. However, it could be moved to a separate
file in qom/.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg
2015-12-23 18:02 ` Paolo Bonzini
@ 2015-12-23 19:25 ` Daniel P. Berrange
0 siblings, 0 replies; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-23 19:25 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Andreas Färber
On Wed, Dec 23, 2015 at 07:02:01PM +0100, Paolo Bonzini wrote:
>
>
> On 22/12/2015 18:24, Daniel P. Berrange wrote:
> > I was a little reluctant to move this 'object_create' method into the
> > qom/ code though, since I hate the idea of the legacy 'QemuOpts' data
> > anywhere near those nice new APIs. I guess I could perhaps just keep the
> > qemu_opts_to_qdict() call in the caller.
>
> I disagree with calling QemuOpts legacy, but I agree that this shouldn't
> be in qom/object_interfaces.c. However, it could be moved to a separate
> file in qom/.
Ok, yeah, I shouldn't have called it legacy really. This was my shorthand
for saying that internal infrastructure APIs should strive to be qapi/qom
based, and QemuOpts should only be used at the very top most level (ie
cli ARGV) and converted to qapi/qom as soon as possible. I think that
my v2 series achieves that goal fairly well.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
` (3 preceding siblings ...)
2015-12-22 11:06 ` [Qemu-devel] [PATCH 4/7] qemu-io: " Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 17:06 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: " Daniel P. Berrange
2015-12-22 11:06 ` [Qemu-devel] [PATCH 7/7] qemu-img: " Daniel P. Berrange
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Currently qemu-io allows an image filename to be passed on the
command line, but 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 --source arg (that is mutually exclusive with a
positional filename arg and -f arg) that accepts a full option
string, as well as the original syntax eg
qemu-io --source driver=http,url=https://127.0.0.1/images,sslverify=off
qemu-io --source https://127.0.0.1/images/centos7.iso
qemu-io --source file=/home/berrange/demo.qcow2
qemu-io --source /home/berrange/demo.qcow2
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-io.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/qemu-io.c b/qemu-io.c
index cf1dac6..fc7f81b 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -373,6 +373,7 @@ static void reenable_tty_echo(void)
enum {
OPTION_OBJECT = 258,
+ OPTION_SOURCE = 259,
};
static QemuOptsList qemu_object_opts = {
@@ -436,6 +437,16 @@ out:
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;
@@ -455,6 +466,7 @@ int main(int argc, char **argv)
{ "cache", 1, NULL, 't' },
{ "trace", 1, NULL, 'T' },
{ "object", 1, NULL, OPTION_OBJECT },
+ { "source", 1, NULL, OPTION_SOURCE },
{ NULL, 0, NULL, 0 }
};
int c;
@@ -531,6 +543,12 @@ int main(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (!qemu_opts_parse_noisily(&file_opts, optarg, false)) {
+ qemu_opts_reset(&file_opts);
+ return 0;
+ }
+ break;
default:
usage(progname);
exit(1);
@@ -572,7 +590,24 @@ int main(int argc, char **argv)
flags |= BDRV_O_RDWR;
}
- if ((argc - optind) == 1) {
+ qopts = qemu_opts_find(&file_opts, NULL);
+ if (qopts) {
+ char *file;
+ if (opts) {
+ error_report("--source and -f are mutually exclusive");
+ exit(1);
+ }
+ if ((argc - optind) == 1) {
+ error_report("--source and filename 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();
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args
2015-12-22 11:06 ` [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args Daniel P. Berrange
@ 2015-12-22 17:06 ` Eric Blake
2015-12-22 17:13 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 17:06 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Currently qemu-io allows an image filename to be passed on the
> command line, but 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
Well, you CAN set the options by specifying the file as:
qemu-io json:{...}
but that's a mouthful to type, so I don't mind adding the new syntax.
>
> This adds a --source arg (that is mutually exclusive with a
> positional filename arg and -f arg) that accepts a full option
> string, as well as the original syntax eg
>
> qemu-io --source driver=http,url=https://127.0.0.1/images,sslverify=off
> qemu-io --source https://127.0.0.1/images/centos7.iso
> qemu-io --source file=/home/berrange/demo.qcow2
> qemu-io --source /home/berrange/demo.qcow2
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qemu-io.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> @@ -572,7 +590,24 @@ int main(int argc, char **argv)
> flags |= BDRV_O_RDWR;
> }
>
> - if ((argc - optind) == 1) {
> + qopts = qemu_opts_find(&file_opts, NULL);
> + if (qopts) {
> + char *file;
> + if (opts) {
> + error_report("--source and -f are mutually exclusive");
> + exit(1);
> + }
> + if ((argc - optind) == 1) {
What if I specify more than one option? Shouldn't this be >=, rather
than ==? Also, the inner () are redundant.
> + error_report("--source and filename 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) {
Pre-existing, but the inner () are redundant.
> openfile(argv[optind], flags, opts);
> }
> command_loop();
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args
2015-12-22 17:06 ` Eric Blake
@ 2015-12-22 17:13 ` Daniel P. Berrange
0 siblings, 0 replies; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 17:13 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 10:06:00AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Currently qemu-io allows an image filename to be passed on the
> > command line, but 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
>
> Well, you CAN set the options by specifying the file as:
> qemu-io json:{...}
>
> but that's a mouthful to type, so I don't mind adding the new syntax.
Yep, but as a user, good luck finding out the syntax you have
to provide for that - AFAIK its not documented in any of our
user facing docs. What everyone knows is the syntax accepted
by the -drive command, so I figure we should allow it everywhere.
I didn't even know about the ability to use json until Kevin
mentioned it to me last time I asked about this :-)
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 6/7] qemu-nbd: allow specifying image as a set of options args
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
` (4 preceding siblings ...)
2015-12-22 11:06 ` [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 17:10 ` Eric Blake
2015-12-22 11:06 ` [Qemu-devel] [PATCH 7/7] qemu-img: " Daniel P. Berrange
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Currently qemu-nbd allows an image filename to be passed on the
command line, but 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 --source arg (that is mutually exclusive with a
positional filename arg and -f arg) that accepts a full option
string, as well as the original syntax eg
qemu-nbd --source driver=http,url=https://127.0.0.1/images,sslverify=off
qemu-nbd --source https://127.0.0.1/images/centos7.iso
qemu-nbd --source file=/home/berrange/demo.qcow2
qemu-nbd --source /home/berrange/demo.qcow2
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-nbd.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 50 insertions(+), 7 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b4b6681..0b9f99e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -49,6 +49,7 @@
#define QEMU_NBD_OPT_DISCARD 3
#define QEMU_NBD_OPT_DETECT_ZEROES 4
#define QEMU_NBD_OPT_OBJECT 5
+#define QEMU_NBD_OPT_SOURCE 6
static NBDExport *exp;
static int verbose;
@@ -387,6 +388,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",
@@ -486,6 +497,7 @@ int main(int argc, char **argv)
{ "persistent", 0, NULL, 't' },
{ "verbose", 0, NULL, 'v' },
{ "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+ { "source", 1, NULL, QEMU_NBD_OPT_SOURCE },
{ NULL, 0, NULL, 0 }
};
int ch;
@@ -657,13 +669,23 @@ int main(int argc, char **argv)
exit(1);
}
break;
+ case QEMU_NBD_OPT_SOURCE:
+ if (srcpath) {
+ errx(EXIT_FAILURE, "--source can only be used once");
+ }
+ if (!qemu_opts_parse_noisily(&file_opts, optarg, true)) {
+ qemu_opts_reset(&file_opts);
+ exit(EXIT_FAILURE);
+ }
+ srcpath = optarg;
+ break;
case '?':
errx(EXIT_FAILURE, "Try `%s --help' for more information.",
argv[0]);
}
}
- if ((argc - optind) != 1) {
+ if ((argc - optind) > 1) {
errx(EXIT_FAILURE, "Invalid number of argument.\n"
"Try `%s --help' for more information.",
argv[0]);
@@ -757,15 +779,36 @@ 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));
+ if (srcpath) {
+ char *file = NULL;
+ opts = qemu_opts_find(&file_opts, NULL);
+ if (fmt) {
+ errx(EXIT_FAILURE, "--source and -f are mutually exclusive");
+ }
+ if ((argc - optind) > 1) {
+ errx(EXIT_FAILURE, "--source and filename are mutually exclusive");
+ }
+ 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));
+ }
+ if ((argc - optind) != 1) {
+ errx(EXIT_FAILURE, "one of --source or filename are expected");
+ }
+
+ srcpath = argv[optind];
+ 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) {
- errx(EXIT_FAILURE, "Failed to blk_new_open '%s': %s", argv[optind],
+ errx(EXIT_FAILURE, "Failed to blk_new_open '%s': %s", srcpath,
error_get_pretty(local_err));
}
bs = blk_bs(blk);
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] qemu-nbd: allow specifying image as a set of options args
2015-12-22 11:06 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: " Daniel P. Berrange
@ 2015-12-22 17:10 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2015-12-22 17:10 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Currently qemu-nbd allows an image filename to be passed on the
> command line, but 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
Well, technically we have:
qemu-nbd json:{...}
but I'm fine with adding the nicer syntax.
>
> This adds a --source arg (that is mutually exclusive with a
> positional filename arg and -f arg) that accepts a full option
> string, as well as the original syntax eg
>
> qemu-nbd --source driver=http,url=https://127.0.0.1/images,sslverify=off
> qemu-nbd --source https://127.0.0.1/images/centos7.iso
> qemu-nbd --source file=/home/berrange/demo.qcow2
> qemu-nbd --source /home/berrange/demo.qcow2
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qemu-nbd.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b4b6681..0b9f99e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -49,6 +49,7 @@
> #define QEMU_NBD_OPT_DISCARD 3
> #define QEMU_NBD_OPT_DETECT_ZEROES 4
> #define QEMU_NBD_OPT_OBJECT 5
> +#define QEMU_NBD_OPT_SOURCE 6
Same comment as earlier about this being safer if it is > 255.
> @@ -486,6 +497,7 @@ int main(int argc, char **argv)
> { "persistent", 0, NULL, 't' },
> { "verbose", 0, NULL, 'v' },
> { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
> + { "source", 1, NULL, QEMU_NBD_OPT_SOURCE },
Same comment as earlier on spelling this as required_argument.
> { NULL, 0, NULL, 0 }
> };
> int ch;
> @@ -657,13 +669,23 @@ int main(int argc, char **argv)
> exit(1);
> }
> break;
> + case QEMU_NBD_OPT_SOURCE:
> + if (srcpath) {
> + errx(EXIT_FAILURE, "--source can only be used once");
Markus has a series getting rid of errx() usage; you'll want to follow suit:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03492.html
> + }
> + if (!qemu_opts_parse_noisily(&file_opts, optarg, true)) {
> + qemu_opts_reset(&file_opts);
> + exit(EXIT_FAILURE);
> + }
> + srcpath = optarg;
> + break;
> case '?':
> errx(EXIT_FAILURE, "Try `%s --help' for more information.",
> argv[0]);
> }
> }
>
> - if ((argc - optind) != 1) {
> + if ((argc - optind) > 1) {
Inner () are redundant.
> errx(EXIT_FAILURE, "Invalid number of argument.\n"
> "Try `%s --help' for more information.",
> argv[0]);
> @@ -757,15 +779,36 @@ 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));
> + if (srcpath) {
> + char *file = NULL;
> + opts = qemu_opts_find(&file_opts, NULL);
> + if (fmt) {
> + errx(EXIT_FAILURE, "--source and -f are mutually exclusive");
> + }
> + if ((argc - optind) > 1) {
and again.
> + errx(EXIT_FAILURE, "--source and filename are mutually exclusive");
More errx() to avoid.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 11:06 [Qemu-devel] [PATCH 0/7] Make qemu-img/qemu-nbd/qemu-io CLI more flexible Daniel P. Berrange
` (5 preceding siblings ...)
2015-12-22 11:06 ` [Qemu-devel] [PATCH 6/7] qemu-nbd: " Daniel P. Berrange
@ 2015-12-22 11:06 ` Daniel P. Berrange
2015-12-22 17:33 ` Eric Blake
6 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 11:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Markus Armbruster, Paolo Bonzini,
Andreas Färber
Currently qemu-img allows an image filename to be passed on the
command line, but 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 --source arg (that is mutually exclusive with a
positional filename arg and -f arg) that accepts a full option
string, as well as the original syntax eg
qemu-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qemu/option.h | 1 +
qemu-img.c | 474 ++++++++++++++++++++++++++++++++++++++++++--------
util/qemu-option.c | 6 +
3 files changed, 407 insertions(+), 74 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 71f5f27..caf5a3b 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -104,6 +104,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
Error **errp);
QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
+QemuOpts *qemu_opts_next(QemuOpts *opts);
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
int fail_if_exists, Error **errp);
void qemu_opts_reset(QemuOptsList *list);
diff --git a/qemu-img.c b/qemu-img.c
index 47f0006..7a6ce81 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -38,6 +38,7 @@
#include "block/blockjob.h"
#include "block/qapi.h"
#include <getopt.h>
+#include <err.h>
#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION QEMU_PKGVERSION \
", Copyright (c) 2004-2008 Fabrice Bellard\n"
@@ -51,6 +52,8 @@ enum {
OPTION_OUTPUT = 256,
OPTION_BACKING_CHAIN = 257,
OPTION_OBJECT = 258,
+ OPTION_SOURCE = 259,
+ OPTION_TARGET = 260,
};
typedef enum OutputFormat {
@@ -172,6 +175,16 @@ static QemuOptsList qemu_object_opts = {
},
};
+static QemuOptsList qemu_source_opts = {
+ .name = "source",
+ .implied_opt_name = "file",
+ .head = QTAILQ_HEAD_INITIALIZER(qemu_source_opts.head),
+ .desc = {
+ { }
+ },
+};
+
+
static int object_create(void *opaque, QemuOpts *opts, Error **errp)
{
Error *err = NULL;
@@ -266,9 +279,31 @@ static int print_block_option_help(const char *filename, const char *fmt)
return 0;
}
-static BlockBackend *img_open(const char *id, const char *filename,
- const char *fmt, int flags,
- bool require_io, bool quiet)
+static BlockBackend *img_open_opts(const char *id,
+ QemuOpts *opts, int flags)
+{
+ QDict *options;
+ Error *local_err = NULL;
+ char *file = NULL;
+ BlockBackend *blk;
+ file = g_strdup(qemu_opt_get(opts, "file"));
+ qemu_opt_unset(opts, "file");
+ options = qemu_opts_to_qdict(opts, NULL);
+ blk = blk_new_open(id, file, NULL, options, flags, &local_err);
+ if (!blk) {
+ error_report("Could not open '%s': %s", file ? file : "",
+ error_get_pretty(local_err));
+ g_free(file);
+ error_free(local_err);
+ return NULL;
+ }
+ g_free(file);
+ return blk;
+}
+
+static BlockBackend *img_open_file(const char *id, const char *filename,
+ const char *fmt, int flags,
+ bool require_io, bool quiet)
{
BlockBackend *blk;
BlockDriverState *bs;
@@ -576,7 +611,7 @@ static int img_check(int argc, char **argv)
{
int c, ret;
OutputFormat output_format = OFORMAT_HUMAN;
- const char *filename, *fmt, *output, *cache;
+ const char *filename = NULL, *fmt, *output, *cache;
BlockBackend *blk;
BlockDriverState *bs;
int fix = 0;
@@ -596,6 +631,7 @@ static int img_check(int argc, char **argv)
{"repair", required_argument, 0, 'r'},
{"output", required_argument, 0, OPTION_OUTPUT},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "hf:r:T:q",
@@ -639,12 +675,29 @@ static int img_check(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename) {
+ errx(EXIT_FAILURE, "--source can only be specified once");
+ }
+ filename = optarg;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
- filename = argv[optind++];
if (output && !strcmp(output, "json")) {
output_format = OFORMAT_JSON;
@@ -667,7 +720,15 @@ static int img_check(int argc, char **argv)
return 1;
}
- blk = img_open("image", filename, fmt, flags, true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ errx(EXIT_FAILURE, "--source and --format are mutually exclusive");
+ }
+ blk = img_open_opts("image", opts, flags);
+ } else {
+ blk = img_open_file("image", filename, fmt, flags, true, quiet);
+ }
if (!blk) {
return 1;
}
@@ -773,7 +834,7 @@ static void run_block_job(BlockJob *job, Error **errp)
static int img_commit(int argc, char **argv)
{
int c, ret, flags;
- const char *filename, *fmt, *cache, *base;
+ const char *filename = NULL, *fmt, *cache, *base;
BlockBackend *blk;
BlockDriverState *bs, *base_bs;
bool progress = false, quiet = false, drop = false;
@@ -789,6 +850,7 @@ static int img_commit(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:ht:b:dpq",
@@ -828,6 +890,17 @@ static int img_commit(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename) {
+ errx(EXIT_FAILURE, "--source can only be specified once");
+ }
+ filename = optarg;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -836,10 +909,16 @@ static int img_commit(int argc, char **argv)
progress = false;
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
- filename = argv[optind++];
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
@@ -854,7 +933,15 @@ static int img_commit(int argc, char **argv)
return 1;
}
- blk = img_open("image", filename, fmt, flags, true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ errx(EXIT_FAILURE, "--source and --format are mutually exclusive");
+ }
+ blk = img_open_opts("image", opts, flags);
+ } else {
+ blk = img_open_file("image", filename, fmt, flags, true, quiet);
+ }
if (!blk) {
return 1;
}
@@ -1088,7 +1175,8 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
*/
static int img_compare(int argc, char **argv)
{
- const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
+ const char *fmt1 = NULL, *fmt2 = NULL, *cache,
+ *filename1 = NULL, *filename2 = NULL;
BlockBackend *blk1, *blk2;
BlockDriverState *bs1, *bs2;
int64_t total_sectors1, total_sectors2;
@@ -1111,6 +1199,7 @@ static int img_compare(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "hf:F:T:pqs",
@@ -1148,6 +1237,20 @@ static int img_compare(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename2) {
+ errx(EXIT_FAILURE, "--source can only be specified twice");
+ } else if (filename1) {
+ filename2 = optarg;
+ } else {
+ filename1 = optarg;
+ }
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -1156,12 +1259,20 @@ static int img_compare(int argc, char **argv)
progress = false;
}
-
- if (optind != argc - 2) {
- error_exit("Expecting two image file names");
+ if (filename1) {
+ if (optind != argc) {
+ error_exit("--source and filenames are mutually exclusive");
+ }
+ if (!filename2) {
+ error_exit("Expecting two --source arguments");
+ }
+ } else {
+ if (optind != argc - 2) {
+ error_exit("Expecting two image file names");
+ }
+ filename1 = argv[optind++];
+ filename2 = argv[optind++];
}
- filename1 = argv[optind++];
- filename2 = argv[optind++];
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
@@ -1180,18 +1291,38 @@ static int img_compare(int argc, char **argv)
goto out3;
}
- blk1 = img_open("image_1", filename1, fmt1, flags, true, quiet);
- if (!blk1) {
- ret = 2;
- goto out3;
- }
- bs1 = blk_bs(blk1);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt1 || fmt2) {
+ error_report("--source and -f or -F are mutuall exclusive");
+ goto out3;
+ }
- blk2 = img_open("image_2", filename2, fmt2, flags, true, quiet);
- if (!blk2) {
- ret = 2;
- goto out2;
+ blk1 = img_open_opts("image_1", opts, flags);
+ if (!blk1) {
+ ret = 2;
+ goto out3;
+ }
+
+ blk2 = img_open_opts("image_2", qemu_opts_next(opts), flags);
+ if (!blk2) {
+ ret = 2;
+ goto out3;
+ }
+ } else {
+ blk1 = img_open_file("image_1", filename1, fmt1, flags, true, quiet);
+ if (!blk1) {
+ ret = 2;
+ goto out3;
+ }
+
+ blk2 = img_open_file("image_2", filename2, fmt2, flags, true, quiet);
+ if (!blk2) {
+ ret = 2;
+ goto out2;
+ }
}
+ bs1 = blk_bs(blk1);
bs2 = blk_bs(blk2);
buf1 = blk_blockalign(blk1, IO_BUF_SIZE);
@@ -1658,10 +1789,11 @@ fail:
static int img_convert(int argc, char **argv)
{
- int c, bs_n, bs_i, compress, cluster_sectors, skip_create;
+ int c, bs_n = 0, bs_i, compress, cluster_sectors, skip_create;
int64_t ret = 0;
int progress = 0, flags, src_flags;
- const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
+ const char *fmt, *out_fmt, *cache, *src_cache,
+ *out_baseimg, *out_filename = NULL;
BlockDriver *drv, *proto_drv;
BlockBackend **blk = NULL, *out_blk = NULL;
BlockDriverState **bs = NULL, *out_bs = NULL;
@@ -1692,6 +1824,7 @@ static int img_convert(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
@@ -1793,6 +1926,14 @@ static int img_convert(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ bs_n++;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -1808,20 +1949,33 @@ static int img_convert(int argc, char **argv)
}
qemu_progress_init(progress, 1.0);
-
- bs_n = argc - optind - 1;
- out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
+ if (!bs_n) {
+ out_filename = (argc - optind - 1) >= 1 ? argv[argc - 1] : NULL;
+ }
if (options && has_help_option(options)) {
ret = print_block_option_help(out_filename, out_fmt);
goto out;
}
- if (bs_n < 1) {
- error_exit("Must specify image file name");
+ if (bs_n) {
+ if (argc > (optind + 1)) {
+ error_exit("--source and filenames are mutually exclusive");
+ }
+ if (argc != (optind + 1)) {
+ error_exit("Must specify target image file name");
+ }
+ if (!bs_n) {
+ error_exit("At least one --source arg is expected with --target");
+ }
+ out_filename = argv[argc - 1];
+ } else {
+ bs_n = argc - optind - 1;
+ if (bs_n < 1) {
+ error_exit("Must specify image file name");
+ }
}
-
if (bs_n > 1 && out_baseimg) {
error_report("-B makes no sense when concatenating multiple input "
"images");
@@ -1843,11 +1997,21 @@ static int img_convert(int argc, char **argv)
bs_sectors = g_new(int64_t, bs_n);
total_sectors = 0;
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
for (bs_i = 0; bs_i < bs_n; bs_i++) {
char *id = bs_n > 1 ? g_strdup_printf("source_%d", bs_i)
: g_strdup("source");
- blk[bs_i] = img_open(id, argv[optind + bs_i], fmt, src_flags,
- true, quiet);
+ if (opts) {
+ if (fmt) {
+ error_report("--source and -f are mutually exclusive");
+ goto out;
+ }
+ blk[bs_i] = img_open_opts(id, opts, src_flags);
+ opts = qemu_opts_next(opts);
+ } else {
+ blk[bs_i] = img_open_file(id, argv[optind + bs_i], fmt, src_flags,
+ true, quiet);
+ }
g_free(id);
if (!blk[bs_i]) {
ret = -1;
@@ -1991,7 +2155,12 @@ static int img_convert(int argc, char **argv)
goto out;
}
- out_blk = img_open("target", out_filename, out_fmt, flags, true, quiet);
+ /* XXX we could allow a --target OPTSRING and then use
+ * img_open_opts here, but then we have trouble with
+ * the bdrv_create() call which takes different params
+ */
+ out_blk = img_open_file("target", out_filename,
+ out_fmt, flags, true, quiet);
if (!out_blk) {
ret = -1;
goto out;
@@ -2158,7 +2327,8 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
* image file. If there was an error a message will have been printed to
* stderr.
*/
-static ImageInfoList *collect_image_info_list(const char *filename,
+static ImageInfoList *collect_image_info_list(QemuOpts *opts,
+ const char *filename,
const char *fmt,
bool chain)
{
@@ -2182,8 +2352,19 @@ static ImageInfoList *collect_image_info_list(const char *filename,
}
g_hash_table_insert(filenames, (gpointer)filename, NULL);
- blk = img_open("image", filename, fmt,
- BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
+ if (opts) {
+ if (fmt) {
+ error_report("--source and -f are mutually exclusive");
+ goto err;
+ }
+ blk = img_open_opts("image", opts,
+ BDRV_O_FLAGS | BDRV_O_NO_BACKING);
+ opts = NULL;
+ } else {
+ blk = img_open_file("image", filename, fmt,
+ BDRV_O_FLAGS | BDRV_O_NO_BACKING,
+ false, false);
+ }
if (!blk) {
goto err;
}
@@ -2232,7 +2413,7 @@ static int img_info(int argc, char **argv)
int c;
OutputFormat output_format = OFORMAT_HUMAN;
bool chain = false;
- const char *filename, *fmt, *output;
+ const char *filename = NULL, *fmt, *output;
ImageInfoList *list;
QemuOpts *opts;
@@ -2246,6 +2427,7 @@ static int img_info(int argc, char **argv)
{"output", required_argument, 0, OPTION_OUTPUT},
{"backing-chain", no_argument, 0, OPTION_BACKING_CHAIN},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:h",
@@ -2274,12 +2456,29 @@ static int img_info(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename) {
+ error_exit("--source can only be specified once");
+ }
+ filename = optarg;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
- filename = argv[optind++];
if (output && !strcmp(output, "json")) {
output_format = OFORMAT_JSON;
@@ -2296,7 +2495,8 @@ static int img_info(int argc, char **argv)
exit(1);
}
- list = collect_image_info_list(filename, fmt, chain);
+ list = collect_image_info_list(qemu_opts_find(&qemu_source_opts, NULL),
+ filename, fmt, chain);
if (!list) {
return 1;
}
@@ -2415,7 +2615,7 @@ static int img_map(int argc, char **argv)
OutputFormat output_format = OFORMAT_HUMAN;
BlockBackend *blk;
BlockDriverState *bs;
- const char *filename, *fmt, *output;
+ const char *filename = NULL, *fmt, *output;
int64_t length;
MapEntry curr = { .length = 0 }, next;
int ret = 0;
@@ -2430,6 +2630,7 @@ static int img_map(int argc, char **argv)
{"format", required_argument, 0, 'f'},
{"output", required_argument, 0, OPTION_OUTPUT},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:h",
@@ -2455,12 +2656,29 @@ static int img_map(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename) {
+ error_exit("--source can only be specified once");
+ }
+ filename = optarg;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind];
}
- filename = argv[optind];
if (output && !strcmp(output, "json")) {
output_format = OFORMAT_JSON;
@@ -2477,7 +2695,15 @@ static int img_map(int argc, char **argv)
exit(1);
}
- blk = img_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ errx(EXIT_FAILURE, "--source and --format are mutually exclusive");
+ }
+ blk = img_open_opts("image", opts, BDRV_O_FLAGS);
+ } else {
+ blk = img_open_file("image", filename, fmt, BDRV_O_FLAGS, true, false);
+ }
if (!blk) {
return 1;
}
@@ -2536,7 +2762,7 @@ static int img_snapshot(int argc, char **argv)
BlockBackend *blk;
BlockDriverState *bs;
QEMUSnapshotInfo sn;
- char *filename, *snapshot_name = NULL;
+ char *filename = NULL, *snapshot_name = NULL;
int c, ret = 0, bdrv_oflags;
int action = 0;
qemu_timeval tv;
@@ -2551,6 +2777,7 @@ static int img_snapshot(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "la:c:d:hq",
@@ -2605,13 +2832,30 @@ static int img_snapshot(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ if (filename) {
+ error_exit("--source can only be specified once");
+ }
+ filename = optarg;
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
- filename = argv[optind++];
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
@@ -2620,7 +2864,12 @@ static int img_snapshot(int argc, char **argv)
}
/* Open the image */
- blk = img_open("image", filename, NULL, bdrv_oflags, true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ blk = img_open_opts("image", opts, bdrv_oflags);
+ } else {
+ blk = img_open_file("image", filename, NULL, bdrv_oflags, true, quiet);
+ }
if (!blk) {
return 1;
}
@@ -2678,7 +2927,7 @@ static int img_rebase(int argc, char **argv)
{
BlockBackend *blk = NULL, *blk_old_backing = NULL, *blk_new_backing = NULL;
BlockDriverState *bs = NULL;
- char *filename;
+ char *filename = NULL;
const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
int c, flags, src_flags, ret;
int unsafe = 0;
@@ -2698,6 +2947,7 @@ static int img_rebase(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
@@ -2741,6 +2991,13 @@ static int img_rebase(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -2748,13 +3005,20 @@ static int img_rebase(int argc, char **argv)
progress = 0;
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
+
if (!unsafe && !out_baseimg) {
error_exit("Must specify backing file (-b) or use unsafe mode (-u)");
}
- filename = argv[optind++];
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
@@ -2785,7 +3049,17 @@ static int img_rebase(int argc, char **argv)
* Ignore the old backing file for unsafe rebase in case we want to correct
* the reference to a renamed or moved backing file.
*/
- blk = img_open("image", filename, fmt, flags, true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ error_report("--source and --format are mutually exclusive");
+ ret = -1;
+ goto out;
+ }
+ blk = img_open_opts("image", opts, flags);
+ } else {
+ blk = img_open_file("image", filename, fmt, flags, true, quiet);
+ }
if (!blk) {
ret = -1;
goto out;
@@ -3017,7 +3291,7 @@ static int img_resize(int argc, char **argv)
{
Error *err = NULL;
int c, ret, relative;
- const char *filename, *fmt, *size;
+ const char *filename = NULL, *fmt, *size;
int64_t n, total_size;
bool quiet = false;
BlockBackend *blk = NULL;
@@ -3053,6 +3327,7 @@ static int img_resize(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "f:hq",
@@ -3078,12 +3353,25 @@ static int img_resize(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ } else {
+ if (optind != argc - 1) {
+ error_exit("Expecting one image file name");
+ }
+ filename = argv[optind++];
}
- filename = argv[optind++];
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
@@ -3118,8 +3406,18 @@ static int img_resize(int argc, char **argv)
n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
qemu_opts_del(param);
- blk = img_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
- true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ error_report("--source and --format are mutually exclusive");
+ ret = -1;
+ goto out;
+ }
+ blk = img_open_opts("image", opts, BDRV_O_FLAGS | BDRV_O_RDWR);
+ } else {
+ blk = img_open_file("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
+ true, quiet);
+ }
if (!blk) {
ret = -1;
goto out;
@@ -3173,7 +3471,7 @@ static int img_amend(int argc, char **argv)
char *options = NULL;
QemuOptsList *create_opts = NULL;
QemuOpts *opts = NULL;
- const char *fmt = NULL, *filename, *cache;
+ const char *fmt = NULL, *filename = NULL, *cache;
int flags;
bool quiet = false, progress = false;
BlockBackend *blk = NULL;
@@ -3185,6 +3483,7 @@ static int img_amend(int argc, char **argv)
static const struct option long_options[] = {
{"help", no_argument, 0, 'h'},
{"object", required_argument, 0, OPTION_OBJECT},
+ {"source", required_argument, 0, OPTION_SOURCE},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, "ho:f:t:pq",
@@ -3231,6 +3530,13 @@ static int img_amend(int argc, char **argv)
exit(1);
}
break;
+ case OPTION_SOURCE:
+ opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
+ optarg, true);
+ if (!opts) {
+ exit(1);
+ }
+ break;
}
}
@@ -3238,6 +3544,12 @@ static int img_amend(int argc, char **argv)
error_exit("Must specify options (-o)");
}
+ if (filename) {
+ if (optind != argc) {
+ error_exit("--source and filename are mutually exclusive");
+ }
+ }
+
if (qemu_opts_foreach(qemu_find_opts("object"),
object_create,
NULL, NULL)) {
@@ -3249,7 +3561,9 @@ static int img_amend(int argc, char **argv)
}
qemu_progress_init(progress, 1.0);
- filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
+ if (!filename) {
+ filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
+ }
if (fmt && has_help_option(options)) {
/* If a format is explicitly specified (and possibly no filename is
* given), print option help here */
@@ -3257,8 +3571,9 @@ static int img_amend(int argc, char **argv)
goto out;
}
- if (optind != argc - 1) {
- error_report("Expecting one image file name");
+ if (!filename &&
+ (optind != argc - 1)) {
+ error_exit("Expecting one image file name");
ret = -1;
goto out;
}
@@ -3270,7 +3585,17 @@ static int img_amend(int argc, char **argv)
goto out;
}
- blk = img_open("image", filename, fmt, flags, true, quiet);
+ opts = qemu_opts_find(&qemu_source_opts, NULL);
+ if (opts) {
+ if (fmt) {
+ error_report("--source and --format are mutually exclusive");
+ ret = -1;
+ goto out;
+ }
+ blk = img_open_opts("image", opts, BDRV_O_FLAGS | BDRV_O_RDWR);
+ } else {
+ blk = img_open_file("image", filename, fmt, flags, true, quiet);
+ }
if (!blk) {
ret = -1;
goto out;
@@ -3368,6 +3693,7 @@ int main(int argc, char **argv)
module_call_init(MODULE_INIT_QOM);
qemu_add_opts(&qemu_object_opts);
+ qemu_add_opts(&qemu_source_opts);
/* find the command */
for (cmd = img_cmds; cmd->name != NULL; cmd++) {
diff --git a/util/qemu-option.c b/util/qemu-option.c
index a50ecea..48e1cc5 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -637,6 +637,12 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
return NULL;
}
+
+QemuOpts *qemu_opts_next(QemuOpts *opts)
+{
+ return QTAILQ_NEXT(opts, next);
+}
+
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
int fail_if_exists, Error **errp)
{
--
2.5.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 11:06 ` [Qemu-devel] [PATCH 7/7] qemu-img: " Daniel P. Berrange
@ 2015-12-22 17:33 ` Eric Blake
2015-12-22 17:42 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 17:33 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Markus Armbruster, qemu-block,
Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 8030 bytes --]
On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Currently qemu-img allows an image filename to be passed on the
> command line, but 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 --source arg (that is mutually exclusive with a
> positional filename arg and -f arg) that accepts a full option
> string, as well as the original syntax eg
>
> qemu-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off
Don't we also need this for destinations and their matching options, for
'compare'(-F), 'convert'(-O) [oh yikes: convert takes more than one
input file - how will we support that?], and 'rebase'(-b/-F)?
The idea is nice, but I don't think we've fully covered the design space.
/me reads ahead
Oh, you DO add a --target, but didn't spell it out in the commit
message...[1]
> ---
> include/qemu/option.h | 1 +
> qemu-img.c | 474 ++++++++++++++++++++++++++++++++++++++++++--------
> util/qemu-option.c | 6 +
> 3 files changed, 407 insertions(+), 74 deletions(-)
And where's the documentation patches, for the man page?
> +++ b/include/qemu/option.h
> @@ -104,6 +104,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> Error **errp);
>
> QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
> +QemuOpts *qemu_opts_next(QemuOpts *opts);
Should exposing this be a separate patch?
> QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
> int fail_if_exists, Error **errp);
While we're touching QemuOpts, should we switch fail_if_exists to bool
(separate patch)?
> void qemu_opts_reset(QemuOptsList *list);
> diff --git a/qemu-img.c b/qemu-img.c
> index 47f0006..7a6ce81 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -38,6 +38,7 @@
> #include "block/blockjob.h"
> #include "block/qapi.h"
> #include <getopt.h>
> +#include <err.h>
No. Markus is trying to get rid of this.
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03492.html
because it does not give consistent error messages (things like
timestamps, for example).
>
> #define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION QEMU_PKGVERSION \
> ", Copyright (c) 2004-2008 Fabrice Bellard\n"
> @@ -51,6 +52,8 @@ enum {
> OPTION_OUTPUT = 256,
> OPTION_BACKING_CHAIN = 257,
> OPTION_OBJECT = 258,
> + OPTION_SOURCE = 259,
> + OPTION_TARGET = 260,
[1]...not mentioned in the commit message.
> +static BlockBackend *img_open_opts(const char *id,
> + QemuOpts *opts, int flags)
> +{
> + QDict *options;
> + Error *local_err = NULL;
> + char *file = NULL;
> + BlockBackend *blk;
> + file = g_strdup(qemu_opt_get(opts, "file"));
> + qemu_opt_unset(opts, "file");
> + options = qemu_opts_to_qdict(opts, NULL);
> + blk = blk_new_open(id, file, NULL, options, flags, &local_err);
> + if (!blk) {
> + error_report("Could not open '%s': %s", file ? file : "",
> + error_get_pretty(local_err));
Markus' series adds the ability to prefix your message rather than
calling error_get_pretty(). Particularly nice if blk_new_open() starts
appending hints to the error. It might be worth waiting for his series
to land and then rebase yours on top of his.
> @@ -1111,6 +1199,7 @@ static int img_compare(int argc, char **argv)
> static const struct option long_options[] = {
> {"help", no_argument, 0, 'h'},
> {"object", required_argument, 0, OPTION_OBJECT},
> + {"source", required_argument, 0, OPTION_SOURCE},
> {0, 0, 0, 0}
> };
> c = getopt_long(argc, argv, "hf:F:T:pqs",
> @@ -1148,6 +1237,20 @@ static int img_compare(int argc, char **argv)
> exit(1);
> }
> break;
> + case OPTION_SOURCE:
> + if (filename2) {
> + errx(EXIT_FAILURE, "--source can only be specified twice");
Weird. Yes, we're reading two files, but --source/--target might be
easier to explain/enforce than --source/--source...
> + } else if (filename1) {
> + filename2 = optarg;
> + } else {
> + filename1 = optarg;
> + }
> + opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
> + optarg, true);
> + if (!opts) {
> + exit(1);
> + }
> + break;
> }
> }
>
> @@ -1156,12 +1259,20 @@ static int img_compare(int argc, char **argv)
> progress = false;
> }
>
> -
> - if (optind != argc - 2) {
> - error_exit("Expecting two image file names");
> + if (filename1) {
> + if (optind != argc) {
> + error_exit("--source and filenames are mutually exclusive");
> + }
> + if (!filename2) {
> + error_exit("Expecting two --source arguments");
> + }
> + } else {
> + if (optind != argc - 2) {
> + error_exit("Expecting two image file names");
> + }
> + filename1 = argv[optind++];
> + filename2 = argv[optind++];
> }
...I think you managed to enforce that there are either two --source or
two positional arguments, and nothing else is valid, but it's not typical.
> - }
> - bs1 = blk_bs(blk1);
> + opts = qemu_opts_find(&qemu_source_opts, NULL);
> + if (opts) {
> + if (fmt1 || fmt2) {
> + error_report("--source and -f or -F are mutuall exclusive");
s/mutuall/mutually/
> @@ -1808,20 +1949,33 @@ static int img_convert(int argc, char **argv)
> }
> qemu_progress_init(progress, 1.0);
>
> -
> - bs_n = argc - optind - 1;
> - out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
> + if (!bs_n) {
> + out_filename = (argc - optind - 1) >= 1 ? argv[argc - 1] : NULL;
> + }
>
> if (options && has_help_option(options)) {
> ret = print_block_option_help(out_filename, out_fmt);
> goto out;
> }
>
> - if (bs_n < 1) {
> - error_exit("Must specify image file name");
> + if (bs_n) {
> + if (argc > (optind + 1)) {
Redundant inner ().
Overall, I'm left wondering whether requiring '--source FOO' vs.
positional 'FOO', and manually enforcing mutual exclusion between the
two, is necessary, or if we could stick with positional. But I guess
the main argument is backwards-compatibility: previously, using
'driver=file,file=/path/to/file' as a filename would try to look in a
relative directory 'driver=file,file=', whereas your proposal of always
using the new '--source' option would make it obvious that we are
expecting to parse a QemuOpts string rather than defaulting to a literal
file name.
On the other hand, the existing positional parameters have allowed
'file:file:with_weird_name' to explicitly specify that we want to use
'./file:with_weird_name' as a relative file in the current directory
(that is, the first 'file:' prefix is sufficient to avoid any
back-compat issues with any other possible change in interpretation to a
prefix), so on that grounds, I'd argue that adding --source is not
necessary, and we can just require users to write
'file:$string_that_might_now_be_QemuOpts' anywhere they used to use
'$string_that_might_now_be_QemuOpts'.
Maybe other block developers have an opinion to offer on whether the
last three patches in this series should be adding a new --source option
as mutually exclusive with positional args, vs. just adding a new
interpretation of the existing mandatory positional arguments?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 17:33 ` Eric Blake
@ 2015-12-22 17:42 ` Daniel P. Berrange
2015-12-22 17:50 ` Eric Blake
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 17:42 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 10:33:48AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Currently qemu-img allows an image filename to be passed on the
> > command line, but 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 --source arg (that is mutually exclusive with a
> > positional filename arg and -f arg) that accepts a full option
> > string, as well as the original syntax eg
> >
> > qemu-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off
>
[snip]
>
> Overall, I'm left wondering whether requiring '--source FOO' vs.
> positional 'FOO', and manually enforcing mutual exclusion between the
> two, is necessary, or if we could stick with positional. But I guess
> the main argument is backwards-compatibility: previously, using
> 'driver=file,file=/path/to/file' as a filename would try to look in a
> relative directory 'driver=file,file=', whereas your proposal of always
> using the new '--source' option would make it obvious that we are
> expecting to parse a QemuOpts string rather than defaulting to a literal
> file name.
>
> On the other hand, the existing positional parameters have allowed
> 'file:file:with_weird_name' to explicitly specify that we want to use
> './file:with_weird_name' as a relative file in the current directory
> (that is, the first 'file:' prefix is sufficient to avoid any
> back-compat issues with any other possible change in interpretation to a
> prefix), so on that grounds, I'd argue that adding --source is not
> necessary, and we can just require users to write
> 'file:$string_that_might_now_be_QemuOpts' anywhere they used to use
> '$string_that_might_now_be_QemuOpts'.
>
> Maybe other block developers have an opinion to offer on whether the
> last three patches in this series should be adding a new --source option
> as mutually exclusive with positional args, vs. just adding a new
> interpretation of the existing mandatory positional arguments?
Yep, back compatibility to avoid breaking any existing possible
filenames was my main motivation for adding '--source'. I agree
it would be nice if we decided that the risk was acceptable
based on what you say above, and thus avoid --source, and just
extend existing positional args.
If block maintainers OK that approach, I'd happily rewrite the
last 3 patches in this series in that manner.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 17:42 ` Daniel P. Berrange
@ 2015-12-22 17:50 ` Eric Blake
2015-12-22 18:07 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 17:50 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]
On 12/22/2015 10:42 AM, Daniel P. Berrange wrote:
>> Overall, I'm left wondering whether requiring '--source FOO' vs.
>> positional 'FOO', and manually enforcing mutual exclusion between the
>> two, is necessary, or if we could stick with positional. But I guess
>> the main argument is backwards-compatibility: previously, using
>> 'driver=file,file=/path/to/file' as a filename would try to look in a
>> relative directory 'driver=file,file=', whereas your proposal of always
>> using the new '--source' option would make it obvious that we are
>> expecting to parse a QemuOpts string rather than defaulting to a literal
>> file name.
>>
>> On the other hand, the existing positional parameters have allowed
>> 'file:file:with_weird_name' to explicitly specify that we want to use
>> './file:with_weird_name' as a relative file in the current directory
>> (that is, the first 'file:' prefix is sufficient to avoid any
>> back-compat issues with any other possible change in interpretation to a
>> prefix), so on that grounds, I'd argue that adding --source is not
>> necessary, and we can just require users to write
>> 'file:$string_that_might_now_be_QemuOpts' anywhere they used to use
>> '$string_that_might_now_be_QemuOpts'.
I guess there's also the issue of literal commas.
Right now, we have:
$ echo hi > a,b
$ qemu-img info a,b
image: a,b
file format: raw
virtual size: 512 (512 bytes)
disk size: 4.0K
$ qemu-img info file:a,b
image: a,b
file format: raw
virtual size: 512 (512 bytes)
disk size: 4.0K
If we magically change things to interpret the positionals as QemuOpts
strings, we'd have a change that:
$ qemu-img info a,b
$ qemu-img info file:a,b
would error out ('a' and 'file:a' are not a known options, and we are
expecting =), but at the same time:
$ qemu-img info a,,b
$ qemu-img info file:a,,b
would start working (because the use of ',,' is the QemuOpts way to
escape a literal ',' that is not separating options packed into the
single argument).
>>
>> Maybe other block developers have an opinion to offer on whether the
>> last three patches in this series should be adding a new --source option
>> as mutually exclusive with positional args, vs. just adding a new
>> interpretation of the existing mandatory positional arguments?
>
> Yep, back compatibility to avoid breaking any existing possible
> filenames was my main motivation for adding '--source'. I agree
> it would be nice if we decided that the risk was acceptable
> based on what you say above, and thus avoid --source, and just
> extend existing positional args.
>
> If block maintainers OK that approach, I'd happily rewrite the
> last 3 patches in this series in that manner.
It may be mid-January before the decision is made, but we've got time
before 2.6 soft freeze. I can wait :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 17:50 ` Eric Blake
@ 2015-12-22 18:07 ` Daniel P. Berrange
2015-12-22 18:10 ` Eric Blake
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-22 18:07 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 10:50:19AM -0700, Eric Blake wrote:
> On 12/22/2015 10:42 AM, Daniel P. Berrange wrote:
>
> >> Overall, I'm left wondering whether requiring '--source FOO' vs.
> >> positional 'FOO', and manually enforcing mutual exclusion between the
> >> two, is necessary, or if we could stick with positional. But I guess
> >> the main argument is backwards-compatibility: previously, using
> >> 'driver=file,file=/path/to/file' as a filename would try to look in a
> >> relative directory 'driver=file,file=', whereas your proposal of always
> >> using the new '--source' option would make it obvious that we are
> >> expecting to parse a QemuOpts string rather than defaulting to a literal
> >> file name.
> >>
> >> On the other hand, the existing positional parameters have allowed
> >> 'file:file:with_weird_name' to explicitly specify that we want to use
> >> './file:with_weird_name' as a relative file in the current directory
> >> (that is, the first 'file:' prefix is sufficient to avoid any
> >> back-compat issues with any other possible change in interpretation to a
> >> prefix), so on that grounds, I'd argue that adding --source is not
> >> necessary, and we can just require users to write
> >> 'file:$string_that_might_now_be_QemuOpts' anywhere they used to use
> >> '$string_that_might_now_be_QemuOpts'.
>
> I guess there's also the issue of literal commas.
>
> Right now, we have:
> $ echo hi > a,b
> $ qemu-img info a,b
> image: a,b
> file format: raw
> virtual size: 512 (512 bytes)
> disk size: 4.0K
> $ qemu-img info file:a,b
> image: a,b
> file format: raw
> virtual size: 512 (512 bytes)
> disk size: 4.0K
>
> If we magically change things to interpret the positionals as QemuOpts
> strings, we'd have a change that:
>
> $ qemu-img info a,b
> $ qemu-img info file:a,b
>
> would error out ('a' and 'file:a' are not a known options, and we are
> expecting =), but at the same time:
>
> $ qemu-img info a,,b
> $ qemu-img info file:a,,b
>
> would start working (because the use of ',,' is the QemuOpts way to
> escape a literal ',' that is not separating options packed into the
> single argument).
A third option would be to keep using positional arguments, but
add a '--source-opts' *boolean* flag to indicate how to interpret
the positional arguments. ie without --source-opts we use the
historic syntax, but with --source-opts, we assume the full QemuOpts
syntax.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 18:07 ` Daniel P. Berrange
@ 2015-12-22 18:10 ` Eric Blake
2015-12-23 16:55 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2015-12-22 18:10 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
[-- Attachment #1: Type: text/plain, Size: 628 bytes --]
On 12/22/2015 11:07 AM, Daniel P. Berrange wrote:
> A third option would be to keep using positional arguments, but
> add a '--source-opts' *boolean* flag to indicate how to interpret
> the positional arguments. ie without --source-opts we use the
> historic syntax, but with --source-opts, we assume the full QemuOpts
> syntax.
Oh, nice compromise. It's relatively discoverable (grep --help output),
preserves back-compat of old scripts, and offers the full power for
clients that want the full power.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-22 18:10 ` Eric Blake
@ 2015-12-23 16:55 ` Daniel P. Berrange
2015-12-23 18:03 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-23 16:55 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Paolo Bonzini, Andreas Färber
On Tue, Dec 22, 2015 at 11:10:27AM -0700, Eric Blake wrote:
> On 12/22/2015 11:07 AM, Daniel P. Berrange wrote:
>
> > A third option would be to keep using positional arguments, but
> > add a '--source-opts' *boolean* flag to indicate how to interpret
> > the positional arguments. ie without --source-opts we use the
> > historic syntax, but with --source-opts, we assume the full QemuOpts
> > syntax.
>
> Oh, nice compromise. It's relatively discoverable (grep --help output),
> preserves back-compat of old scripts, and offers the full power for
> clients that want the full power.
I've implemented this now and it makes the patches soooo much simpler
too, so an added win.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-23 16:55 ` Daniel P. Berrange
@ 2015-12-23 18:03 ` Paolo Bonzini
2015-12-23 19:23 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2015-12-23 18:03 UTC (permalink / raw)
To: Daniel P. Berrange, Eric Blake
Cc: Kevin Wolf, Andreas Färber, qemu-devel, qemu-block,
Markus Armbruster
On 23/12/2015 17:55, Daniel P. Berrange wrote:
>>> > > A third option would be to keep using positional arguments, but
>>> > > add a '--source-opts' *boolean* flag to indicate how to interpret
>>> > > the positional arguments. ie without --source-opts we use the
>>> > > historic syntax, but with --source-opts, we assume the full QemuOpts
>>> > > syntax.
>> >
>> > Oh, nice compromise. It's relatively discoverable (grep --help output),
>> > preserves back-compat of old scripts, and offers the full power for
>> > clients that want the full power.
> I've implemented this now and it makes the patches soooo much simpler
> too, so an added win.
Hmm, looks like I'm a bit late, but here's another possibility: making
--source-options (aka -o) take an argument, and if you specify both
--source-options and the positional argument, the latter is added as a
"file" key.
This way you can do "qemu-img info --object secret... -o secret=foo
iscsi://foo/".
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-23 18:03 ` Paolo Bonzini
@ 2015-12-23 19:23 ` Daniel P. Berrange
2015-12-23 20:20 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-23 19:23 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Andreas Färber
On Wed, Dec 23, 2015 at 07:03:34PM +0100, Paolo Bonzini wrote:
>
>
> On 23/12/2015 17:55, Daniel P. Berrange wrote:
> >>> > > A third option would be to keep using positional arguments, but
> >>> > > add a '--source-opts' *boolean* flag to indicate how to interpret
> >>> > > the positional arguments. ie without --source-opts we use the
> >>> > > historic syntax, but with --source-opts, we assume the full QemuOpts
> >>> > > syntax.
> >> >
> >> > Oh, nice compromise. It's relatively discoverable (grep --help output),
> >> > preserves back-compat of old scripts, and offers the full power for
> >> > clients that want the full power.
> > I've implemented this now and it makes the patches soooo much simpler
> > too, so an added win.
>
> Hmm, looks like I'm a bit late, but here's another possibility: making
> --source-options (aka -o) take an argument, and if you specify both
> --source-options and the positional argument, the latter is added as a
> "file" key.
>
> This way you can do "qemu-img info --object secret... -o secret=foo
> iscsi://foo/".
This gets somewhat messy for the commands which needs to accept more
than one filename, eg 'compare' (2 files) and 'convert' (any number
of files)'. eg if you have 3 positional file names and want to set
an option on the 2nd file, '-o foo=bar' is not really going to say
which positional file it applies to. The style I just implemented
with the boolean '--image-opts' flag avoids this messiness nicely.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-23 19:23 ` Daniel P. Berrange
@ 2015-12-23 20:20 ` Paolo Bonzini
2015-12-24 10:04 ` Daniel P. Berrange
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2015-12-23 20:20 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Andreas Färber
----- Original Message -----
> From: "Daniel P. Berrange" <berrange@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Eric Blake" <eblake@redhat.com>, "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org,
> "Markus Armbruster" <armbru@redhat.com>, "Andreas Färber" <afaerber@suse.de>
> Sent: Wednesday, December 23, 2015 8:23:13 PM
> Subject: Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
>
> On Wed, Dec 23, 2015 at 07:03:34PM +0100, Paolo Bonzini wrote:
> >
> >
> > On 23/12/2015 17:55, Daniel P. Berrange wrote:
> > >>> > > A third option would be to keep using positional arguments, but
> > >>> > > add a '--source-opts' *boolean* flag to indicate how to interpret
> > >>> > > the positional arguments. ie without --source-opts we use the
> > >>> > > historic syntax, but with --source-opts, we assume the full
> > >>> > > QemuOpts
> > >>> > > syntax.
> > >> >
> > >> > Oh, nice compromise. It's relatively discoverable (grep --help
> > >> > output),
> > >> > preserves back-compat of old scripts, and offers the full power for
> > >> > clients that want the full power.
> > > I've implemented this now and it makes the patches soooo much simpler
> > > too, so an added win.
> >
> > Hmm, looks like I'm a bit late, but here's another possibility: making
> > --source-options (aka -o) take an argument, and if you specify both
> > --source-options and the positional argument, the latter is added as a
> > "file" key.
> >
> > This way you can do "qemu-img info --object secret... -o secret=foo
> > iscsi://foo/".
>
> This gets somewhat messy for the commands which needs to accept more
> than one filename, eg 'compare' (2 files) and 'convert' (any number
> of files)'. eg if you have 3 positional file names and want to set
> an option on the 2nd file, '-o foo=bar' is not really going to say
> which positional file it applies to.
That would be -o/-O, like -f/-F.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
2015-12-23 20:20 ` Paolo Bonzini
@ 2015-12-24 10:04 ` Daniel P. Berrange
0 siblings, 0 replies; 29+ messages in thread
From: Daniel P. Berrange @ 2015-12-24 10:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, qemu-block, qemu-devel, Markus Armbruster,
Andreas Färber
On Wed, Dec 23, 2015 at 03:20:18PM -0500, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: "Eric Blake" <eblake@redhat.com>, "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, qemu-devel@nongnu.org,
> > "Markus Armbruster" <armbru@redhat.com>, "Andreas Färber" <afaerber@suse.de>
> > Sent: Wednesday, December 23, 2015 8:23:13 PM
> > Subject: Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args
> >
> > On Wed, Dec 23, 2015 at 07:03:34PM +0100, Paolo Bonzini wrote:
> > >
> > >
> > > On 23/12/2015 17:55, Daniel P. Berrange wrote:
> > > >>> > > A third option would be to keep using positional arguments, but
> > > >>> > > add a '--source-opts' *boolean* flag to indicate how to interpret
> > > >>> > > the positional arguments. ie without --source-opts we use the
> > > >>> > > historic syntax, but with --source-opts, we assume the full
> > > >>> > > QemuOpts
> > > >>> > > syntax.
> > > >> >
> > > >> > Oh, nice compromise. It's relatively discoverable (grep --help
> > > >> > output),
> > > >> > preserves back-compat of old scripts, and offers the full power for
> > > >> > clients that want the full power.
> > > > I've implemented this now and it makes the patches soooo much simpler
> > > > too, so an added win.
> > >
> > > Hmm, looks like I'm a bit late, but here's another possibility: making
> > > --source-options (aka -o) take an argument, and if you specify both
> > > --source-options and the positional argument, the latter is added as a
> > > "file" key.
> > >
> > > This way you can do "qemu-img info --object secret... -o secret=foo
> > > iscsi://foo/".
> >
> > This gets somewhat messy for the commands which needs to accept more
> > than one filename, eg 'compare' (2 files) and 'convert' (any number
> > of files)'. eg if you have 3 positional file names and want to set
> > an option on the 2nd file, '-o foo=bar' is not really going to say
> > which positional file it applies to.
>
> That would be -o/-O, like -f/-F.
That only works for two files, unless you keep inventing yet more
characters for the 3rd, 4th, ... file - 'convert' takes an arbitrary
number of files.
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread