* [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del
@ 2013-12-10 17:00 Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
These allow hotplugging (and hot-unplugging without leaking an object)
virtio-rng devices. They can also be used for memory hotplug.
Paolo Bonzini (5):
rng: initialize file descriptor to -1
qom: fix leak for objects created with -object
qom: catch errors in object_property_add_child
monitor: add object-add (QMP) and object_add (HMP) command
monitor: add object-del (QMP) and object_del (HMP) command
backends/rng-random.c | 4 +--
hmp-commands.hx | 28 ++++++++++++++++++
hmp.c | 67 ++++++++++++++++++++++++++++++++++++++++++
hmp.h | 2 ++
include/monitor/monitor.h | 3 ++
include/qapi/visitor.h | 3 +-
include/qemu/typedefs.h | 2 ++
qapi-schema.json | 34 ++++++++++++++++++++++
qmp-commands.hx | 51 ++++++++++++++++++++++++++++++++
qmp.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
qom/object.c | 9 ++++--
vl.c | 3 +-
12 files changed, 273 insertions(+), 7 deletions(-)
--
1.8.4.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
@ 2013-12-10 17:00 ` Paolo Bonzini
2013-12-11 23:14 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object Paolo Bonzini
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
The file descriptor is never initialized to -1, which makes rng-random
close stdin if an object is created and immediately destroyed. If we
change it to -1, we also need to protect qemu_set_fd_handler from
receiving a bogus file descriptor.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
backends/rng-random.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/backends/rng-random.c b/backends/rng-random.c
index 68dfc8a..136499d 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -123,15 +123,15 @@ static void rng_random_init(Object *obj)
NULL);
s->filename = g_strdup("/dev/random");
+ s->fd = -1;
}
static void rng_random_finalize(Object *obj)
{
RndRandom *s = RNG_RANDOM(obj);
- qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
-
if (s->fd != -1) {
+ qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
qemu_close(s->fd);
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
@ 2013-12-10 17:00 ` Paolo Bonzini
2013-12-11 23:15 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child Paolo Bonzini
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
The object must be unref-ed when its variable goes out of scope.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
vl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 8d5d874..6917fd1 100644
--- a/vl.c
+++ b/vl.c
@@ -2807,12 +2807,13 @@ static int object_create(QemuOpts *opts, void *opaque)
obj = object_new(type);
if (qemu_opt_foreach(opts, object_set_property, obj, 1) < 0) {
+ object_unref(obj);
return -1;
}
object_property_add_child(container_get(object_get_root(), "/objects"),
id, obj, NULL);
-
+ object_unref(obj);
return 0;
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object Paolo Bonzini
@ 2013-12-10 17:00 ` Paolo Bonzini
2013-12-11 23:16 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command Paolo Bonzini
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qom/object.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index fc19cf6..68fe07a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -988,17 +988,22 @@ static void object_finalize_child_property(Object *obj, const char *name,
void object_property_add_child(Object *obj, const char *name,
Object *child, Error **errp)
{
+ Error *local_err = NULL;
gchar *type;
type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
object_property_add(obj, name, type, object_get_child_property,
- NULL, object_finalize_child_property, child, errp);
-
+ NULL, object_finalize_child_property, child, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ goto out;
+ }
object_ref(child);
g_assert(child->parent == NULL);
child->parent = obj;
+out:
g_free(type);
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
` (2 preceding siblings ...)
2013-12-10 17:00 ` [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child Paolo Bonzini
@ 2013-12-10 17:00 ` Paolo Bonzini
2013-12-10 18:00 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del " Paolo Bonzini
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
Add two commands that are the monitor counterparts of -object. The commands
have the same Visitor-based implementation, but use different kinds of
visitors so that the HMP command has a DWIM string-based syntax, while
the QMP variant accepts a stricter JSON-based properties dictionary.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands.hx | 14 +++++++++++
hmp.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 1 +
include/monitor/monitor.h | 3 +++
include/qapi/visitor.h | 3 +--
include/qemu/typedefs.h | 2 ++
qapi-schema.json | 20 +++++++++++++++
qmp-commands.hx | 27 +++++++++++++++++++++
qmp.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 188 insertions(+), 2 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index caae5ad..0563b33 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1243,6 +1243,20 @@ STEXI
Remove host network device.
ETEXI
+ {
+ .name = "object_add",
+ .args_type = "object:O",
+ .params = "[qom-type=]type,id=str[,prop=value][,...]",
+ .help = "create QOM object",
+ .mhandler.cmd = hmp_object_add,
+ },
+
+STEXI
+@item object_add
+@findex object_add
+Create QOM object.
+ETEXI
+
#ifdef CONFIG_SLIRP
{
.name = "hostfwd_add",
diff --git a/hmp.c b/hmp.c
index 32ee285..a1669ab 100644
--- a/hmp.c
+++ b/hmp.c
@@ -21,6 +21,7 @@
#include "qmp-commands.h"
#include "qemu/sockets.h"
#include "monitor/monitor.h"
+#include "qapi/opts-visitor.h"
#include "ui/console.h"
#include "block/qapi.h"
#include "qemu-io.h"
@@ -1354,6 +1355,63 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+void hmp_object_add(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+ QemuOpts *opts;
+ char *type = NULL;
+ char *id = NULL;
+ void *dummy = NULL;
+ OptsVisitor *ov;
+ QDict *pdict;
+
+ opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
+ if (error_is_set(&err)) {
+ goto out;
+ }
+
+ ov = opts_visitor_new(opts);
+ pdict = qdict_clone_shallow(qdict);
+
+ visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+ if (error_is_set(&err)) {
+ goto out_clean;
+ }
+
+ qdict_del(pdict, "qom-type");
+ visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+ if (error_is_set(&err)) {
+ goto out_clean;
+ }
+
+ qdict_del(pdict, "id");
+ visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+ if (error_is_set(&err)) {
+ goto out_clean;
+ }
+
+ object_add(type, id, pdict, opts_get_visitor(ov), &err);
+ if (error_is_set(&err)) {
+ goto out_clean;
+ }
+ visit_end_struct(opts_get_visitor(ov), &err);
+ if (error_is_set(&err)) {
+ qmp_object_del(id, NULL);
+ }
+
+out_clean:
+ opts_visitor_cleanup(ov);
+
+ QDECREF(pdict);
+ qemu_opts_del(opts);
+ g_free(id);
+ g_free(type);
+ g_free(dummy);
+
+out:
+ hmp_handle_error(mon, &err);
+}
+
void hmp_getfd(Monitor *mon, const QDict *qdict)
{
const char *fdname = qdict_get_str(qdict, "fdname");
diff --git a/hmp.h b/hmp.h
index 54cf71f..521449b 100644
--- a/hmp.h
+++ b/hmp.h
@@ -89,5 +89,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
void hmp_qemu_io(Monitor *mon, const QDict *qdict);
+void hmp_object_add(Monitor *mon, const QDict *qdict);
#endif
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 10fa0e3..22d8b8f 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -93,6 +93,9 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
+int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret);
+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,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 48a2a2e..29da211 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -13,6 +13,7 @@
#ifndef QAPI_VISITOR_CORE_H
#define QAPI_VISITOR_CORE_H
+#include "qemu/typedefs.h"
#include "qapi/qmp/qobject.h"
#include "qapi/error.h"
#include <stdlib.h>
@@ -26,8 +27,6 @@ typedef struct GenericList
struct GenericList *next;
} GenericList;
-typedef struct Visitor Visitor;
-
void visit_start_handle(Visitor *v, void **obj, const char *kind,
const char *name, Error **errp);
void visit_end_handle(Visitor *v, Error **errp);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index a4c1b84..4524496 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -10,6 +10,8 @@ typedef struct QEMUBH QEMUBH;
typedef struct AioContext AioContext;
+typedef struct Visitor Visitor;
+
struct Monitor;
typedef struct Monitor Monitor;
typedef struct MigrationParams MigrationParams;
diff --git a/qapi-schema.json b/qapi-schema.json
index 83fa485..111463b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2759,6 +2759,26 @@
{ 'command': 'netdev_del', 'data': {'id': 'str'} }
##
+# @object_add:
+#
+# Create a QOM object.
+#
+# @qom-type: the class name for the object to be created
+#
+# @id: the name of the new object
+#
+# @props: #optional a list of properties to be passed to the backend
+#
+# Returns: Nothing on success
+# Error if @qom-type is not a valid class name
+#
+# Since: 2.0
+##
+{ 'command': 'object_add',
+ 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
+ 'gen': 'no' }
+
+##
# @NetdevNoneOptions
#
# Use it alone to have zero network devices.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..d09ea53 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -879,6 +879,33 @@ Example:
EQMP
{
+ .name = "object-add",
+ .args_type = "qom-type:s,id:s,props:q?",
+ .mhandler.cmd_new = qmp_object_add,
+ },
+
+SQMP
+object-add
+----------
+
+Create QOM object.
+
+Arguments:
+
+- "qom-type": the object's QOM type, i.e. the class name (json-string)
+- "id": the object's ID, must be unique (json-string)
+- "props": a dictionary of object property values (optional, json-dict)
+
+Example:
+
+-> { "execute": "object-add", "arguments": { "qom-type": "rng-random", "id": "rng1",
+ "props": { "filename": "/dev/hwrng" } } }
+<- { "return": {} }
+
+EQMP
+
+
+ {
.name = "block_resize",
.args_type = "device:B,size:o",
.mhandler.cmd_new = qmp_marshal_input_block_resize,
diff --git a/qmp.c b/qmp.c
index 4c149b3..255c55f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -24,6 +24,8 @@
#include "hw/qdev.h"
#include "sysemu/blockdev.h"
#include "qom/qom-qobject.h"
+#include "qapi/qmp/qobject.h"
+#include "qapi/qmp-input-visitor.h"
#include "hw/boards.h"
NameInfo *qmp_query_name(Error **errp)
@@ -529,3 +531,63 @@ void qmp_add_client(const char *protocol, const char *fdname,
error_setg(errp, "protocol '%s' is invalid", protocol);
close(fd);
}
+
+void object_add(const char *type, const char *id, const QDict *qdict,
+ Visitor *v, Error **errp)
+{
+ Object *obj;
+ const QDictEntry *e;
+ Error *local_err = NULL;
+
+ if (!object_class_by_name(type)) {
+ error_setg(errp, "invalid class name");
+ 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 (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ object_unref(obj);
+ return;
+ }
+ }
+ }
+
+ object_property_add_child(container_get(object_get_root(), "/objects"),
+ id, obj, errp);
+ object_unref(obj);
+}
+
+int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+ const char *type = qdict_get_str(qdict, "qom-type");
+ const char *id = qdict_get_str(qdict, "id");
+ QObject *props = qdict_get(qdict, "props");
+ const QDict *pdict = NULL;
+ Error *local_err = NULL;
+ QmpInputVisitor *qiv;
+
+ if (props) {
+ pdict = qobject_to_qdict(props);
+ if (!pdict) {
+ error_set(&local_err, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
+ goto out;
+ }
+ }
+
+ qiv = qmp_input_visitor_new(props);
+ object_add(type, id, pdict, qmp_input_get_visitor(qiv), &local_err);
+ qmp_input_visitor_cleanup(qiv);
+
+out:
+ if (local_err) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -1;
+ }
+
+ return 0;
+}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del (HMP) command
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
` (3 preceding siblings ...)
2013-12-10 17:00 ` [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command Paolo Bonzini
@ 2013-12-10 17:00 ` Paolo Bonzini
2013-12-10 18:01 ` Eric Blake
2013-12-12 15:43 ` [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Igor Mammedov
2013-12-16 20:03 ` Luiz Capitulino
6 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
These two commands invoke the "unparent" method of Object.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands.hx | 14 ++++++++++++++
hmp.c | 9 +++++++++
hmp.h | 1 +
qapi-schema.json | 14 ++++++++++++++
qmp-commands.hx | 24 ++++++++++++++++++++++++
qmp.c | 12 ++++++++++++
6 files changed, 74 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0563b33..cb5aa87 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1257,6 +1257,20 @@ STEXI
Create QOM object.
ETEXI
+ {
+ .name = "object_del",
+ .args_type = "id:s",
+ .params = "id",
+ .help = "destroy QOM object",
+ .mhandler.cmd = hmp_object_del,
+ },
+
+STEXI
+@item object_del
+@findex object_del
+Destroy QOM object.
+ETEXI
+
#ifdef CONFIG_SLIRP
{
.name = "hostfwd_add",
diff --git a/hmp.c b/hmp.c
index a1669ab..9f4c9b3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1622,3 +1622,12 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+
+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);
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 521449b..94264d6 100644
--- a/hmp.h
+++ b/hmp.h
@@ -90,5 +90,6 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
void hmp_qemu_io(Monitor *mon, const QDict *qdict);
void hmp_object_add(Monitor *mon, const QDict *qdict);
+void hmp_object_del(Monitor *mon, const QDict *qdict);
#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 111463b..8ca4d0c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2779,6 +2779,20 @@
'gen': 'no' }
##
+# @object_del:
+#
+# Remove a QOM object.
+#
+# @id: the name of the QOM object to remove
+#
+# Returns: Nothing on success
+# Error if @id is not a valid id for a QOM object
+#
+# Since: 2.0
+##
+{ 'command': 'object_del', 'data': {'id': 'str'} }
+
+##
# @NetdevNoneOptions
#
# Use it alone to have zero network devices.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d09ea53..02cc815 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -904,6 +904,30 @@ Example:
EQMP
+ {
+ .name = "object-del",
+ .args_type = "id:s",
+ .mhandler.cmd_new = qmp_marshal_input_object_del,
+ },
+
+SQMP
+object-del
+----------
+
+Remove QOM object.
+
+Arguments:
+
+- "id": the object's ID (json-string)
+
+Example:
+
+-> { "execute": "object-del", "arguments": { "id": "rng1" } }
+<- { "return": {} }
+
+
+EQMP
+
{
.name = "block_resize",
diff --git a/qmp.c b/qmp.c
index 255c55f..0d51e44 100644
--- a/qmp.c
+++ b/qmp.c
@@ -591,3 +591,16 @@ out:
return 0;
}
+
+void qmp_object_del(const char *id, Error **errp)
+{
+ Object *obj;
+
+ obj = object_resolve_path_component(container_get(object_get_root(), "/objects"),
+ id);
+ if (!obj) {
+ error_setg(errp, "object id not found");
+ return;
+ }
+ object_unparent(obj);
+}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-10 17:00 ` [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command Paolo Bonzini
@ 2013-12-10 18:00 ` Eric Blake
2013-12-10 18:15 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2013-12-10 18:00 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]
On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> Add two commands that are the monitor counterparts of -object. The commands
> have the same Visitor-based implementation, but use different kinds of
> visitors so that the HMP command has a DWIM string-based syntax, while
> the QMP variant accepts a stricter JSON-based properties dictionary.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -2759,6 +2759,26 @@
> { 'command': 'netdev_del', 'data': {'id': 'str'} }
>
> ##
> +# @object_add:
I'd prefer object-add for the QMP name (particularly since that's what
you called it in your commit message).
> +#
> +# Create a QOM object.
> +#
> +# @qom-type: the class name for the object to be created
> +#
> +# @id: the name of the new object
> +#
> +# @props: #optional a list of properties to be passed to the backend
s/list/dictionary/
> +#
> +# Returns: Nothing on success
> +# Error if @qom-type is not a valid class name
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'object_add',
Again, object-add
> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
> + 'gen': 'no' }
This feels VERY open-coded. No where else in qapi-schema do we have
'dict' as a type; using it violates all sorts of type-safety (which, I
guess, is the point), making it impossible to introspect what keys are
valid for use in the "props":{...} dictionary. Do we really want to
play this fast and loose with the type system, or should we try harder
to make this a robust self-describing union of types?
That is, why can't we have object-add use a discriminated union, where
qom-type is the discriminator, and where props is an appropriate JSON
struct type that corresponds to the branch of the union, so that we get
full introspection on the set of valid keys to put in props for any
given qom-type?
--
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: 621 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del (HMP) command
2013-12-10 17:00 ` [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del " Paolo Bonzini
@ 2013-12-10 18:01 ` Eric Blake
2013-12-10 18:17 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2013-12-10 18:01 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> These two commands invoke the "unparent" method of Object.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -2779,6 +2779,20 @@
> 'gen': 'no' }
>
> ##
> +# @object_del:
object-del, to match your commit message
> +#
> +# Remove a QOM object.
> +#
> +# @id: the name of the QOM object to remove
> +#
> +# Returns: Nothing on success
> +# Error if @id is not a valid id for a QOM object
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'object_del', 'data': {'id': 'str'} }
again, object-del
> +
> +-> { "execute": "object-del", "arguments": { "id": "rng1" } }
> +<- { "return": {} }
and fixing the json will make your example correct :)
--
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: 621 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-10 18:00 ` Eric Blake
@ 2013-12-10 18:15 ` Paolo Bonzini
2013-12-13 2:55 ` Wenchao Xia
2013-12-16 20:02 ` Luiz Capitulino
0 siblings, 2 replies; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 18:15 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, imammedo, akong, qemu-devel, armbru
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 10/12/2013 19:00, Eric Blake ha scritto:
>>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
>>> + 'gen': 'no' }
>
> This feels VERY open-coded. No where else in qapi-schema do we
> have 'dict' as a type
Yes, in fact the "data" field is entirely skipped by the code
generator (that's 'gen':'no').
> ; using it violates all sorts of type-safety (which, I guess, is
> the point), making it impossible to introspect what keys are valid
> for use in the "props":{...} dictionary. Do we really want to
> play this fast and loose with the type system, or should we try
> harder to make this a robust self-describing union of types?
>
> That is, why can't we have object-add use a discriminated union,
> where qom-type is the discriminator, and where props is an
> appropriate JSON struct type that corresponds to the branch of the
> union, so that we get full introspection on the set of valid keys
> to put in props for any given qom-type?
The point of "props" is passing arbitrary data to a QOM object. We
should indeed have introspection for QOM objects, where each QOM class
name can be introspected separately. However, the union of all
possible QOM objects need not have a "C struct" representation.
Thanks for your review!
Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSp1opAAoJEBvWZb6bTYbyTBoP/RJVnEPZB/JyPH3ybHWpPdAs
3Khn04tSLTsgjmoLBdNwnIDgLQeS2nJYoASVrhIBDhA+heKWgeMH/NGvjB4t8Sug
szVUEmzyeLPEeOydyBX4AfG4yFK39ds3iVjZbJlqQ6Jw50KznIX7mJdgcPiL9ZgQ
PJjvFZ1HGqpNXYMhOPIPgkVEOuN7Z1I9Rf+gyyT2zggn+2Kmo7qs3t5sM1HHbA96
3dPV0PLwieDbqok5RpPFgHAQvrlueMiDEb9yBibfqQ/7blTvJ7tZOpaoXw+9ZddA
zOhBnl9O8vnSM7H+uGVulBtwAXJ4HzSeBIlJ03F5Jln57fmRvfqmhB2ewYlx1pJ3
oYmlyOnLhtVYRqWzry4DrqmewpB19BbHsEbo/9OsCSkfwCLeYQss0S5yeKZlm3GG
LO7zI1iUYulUJJAWbJjoO1MklVNLG9NFYhQkSD4x7mRA9UoYFcGPqK8A5m8XVSkE
APV9di6igTGDA9XvYVxYHZhXgnJ2RzdDXjLJU/gh59INzxaaJaAHM/ye4q0YjAN3
ywzmTnEdfPn4ZsTpnPUMhBIQs2bGMjq2mYoHOMVlkwvuyA8yFsOlFz4fAql/RjJQ
fwJCmJdxgxeRXyqkHOF4szUtTKJI6Pn80vxrMvVTNlPv2vw+AJmMW7LtUVXCbQlg
E/3heznvclCSoKNjrbuo
=BRjp
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del (HMP) command
2013-12-10 18:01 ` Eric Blake
@ 2013-12-10 18:17 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-10 18:17 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, imammedo, akong, qemu-devel, armbru
Il 10/12/2013 19:01, Eric Blake ha scritto:
> On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
>> These two commands invoke the "unparent" method of Object.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>
>> +++ b/qapi-schema.json
>> @@ -2779,6 +2779,20 @@
>> 'gen': 'no' }
>>
>> ##
>> +# @object_del:
>
> object-del, to match your commit message
>
>> +#
>> +# Remove a QOM object.
>> +#
>> +# @id: the name of the QOM object to remove
>> +#
>> +# Returns: Nothing on success
>> +# Error if @id is not a valid id for a QOM object
>> +#
>> +# Since: 2.0
>> +##
>> +{ 'command': 'object_del', 'data': {'id': 'str'} }
>
> again, object-del
>
>> +
>> +-> { "execute": "object-del", "arguments": { "id": "rng1" } }
>> +<- { "return": {} }
>
> and fixing the json will make your example correct :)
It actually worked and was tested, :) because 'command' is only used to
generate the marshaling function name (qmp_marshal_input_object_del).
qmp-commands.hx used the right name, and that's enough.
Of course this doesn't mean qapi-schema.json shouldn't be fixed.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
@ 2013-12-11 23:14 ` Eric Blake
0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2013-12-11 23:14 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 614 bytes --]
On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> The file descriptor is never initialized to -1, which makes rng-random
> close stdin if an object is created and immediately destroyed. If we
> change it to -1, we also need to protect qemu_set_fd_handler from
> receiving a bogus file descriptor.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> backends/rng-random.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
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: 621 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object
2013-12-10 17:00 ` [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object Paolo Bonzini
@ 2013-12-11 23:15 ` Eric Blake
0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2013-12-11 23:15 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 405 bytes --]
On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> The object must be unref-ed when its variable goes out of scope.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> vl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
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: 621 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child
2013-12-10 17:00 ` [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child Paolo Bonzini
@ 2013-12-11 23:16 ` Eric Blake
0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2013-12-11 23:16 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: imammedo, akong, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 348 bytes --]
On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qom/object.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
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: 621 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
` (4 preceding siblings ...)
2013-12-10 17:00 ` [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del " Paolo Bonzini
@ 2013-12-12 15:43 ` Igor Mammedov
2013-12-16 20:03 ` Luiz Capitulino
6 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2013-12-12 15:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: lcapitulino, akong, qemu-devel, armbru
On Tue, 10 Dec 2013 18:00:23 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> These allow hotplugging (and hot-unplugging without leaking an object)
> virtio-rng devices. They can also be used for memory hotplug.
>
> Paolo Bonzini (5):
> rng: initialize file descriptor to -1
> qom: fix leak for objects created with -object
> qom: catch errors in object_property_add_child
> monitor: add object-add (QMP) and object_add (HMP) command
> monitor: add object-del (QMP) and object_del (HMP) command
>
> backends/rng-random.c | 4 +--
> hmp-commands.hx | 28 ++++++++++++++++++
> hmp.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> hmp.h | 2 ++
> include/monitor/monitor.h | 3 ++
> include/qapi/visitor.h | 3 +-
> include/qemu/typedefs.h | 2 ++
> qapi-schema.json | 34 ++++++++++++++++++++++
> qmp-commands.hx | 51 ++++++++++++++++++++++++++++++++
> qmp.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
> qom/object.c | 9 ++++--
> vl.c | 3 +-
> 12 files changed, 273 insertions(+), 7 deletions(-)
>
With s/object_add/object-add/ and s/object_del/object-del/ in 4-5/5 fixed
Reviewed-By: Igor Mammedov <imammedo@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-10 18:15 ` Paolo Bonzini
@ 2013-12-13 2:55 ` Wenchao Xia
2013-12-13 12:19 ` Paolo Bonzini
2013-12-16 20:02 ` Luiz Capitulino
1 sibling, 1 reply; 22+ messages in thread
From: Wenchao Xia @ 2013-12-13 2:55 UTC (permalink / raw)
To: Paolo Bonzini, Eric Blake
Cc: imammedo, akong, qemu-devel, armbru, lcapitulino
于 2013/12/11 2:15, Paolo Bonzini 写道:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 10/12/2013 19:00, Eric Blake ha scritto:
>>>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
>>>> + 'gen': 'no' }
>>
>> This feels VERY open-coded. No where else in qapi-schema do we
>> have 'dict' as a type
>
> Yes, in fact the "data" field is entirely skipped by the code
> generator (that's 'gen':'no').
>
Could we use hmp_object_add()->qmp_object_add()->object_add() code path,
instead of hmp_object_add()->object_add(),qmp_object_add()->object_add()?
Would skipping by generator brings some difficult to it?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-13 2:55 ` Wenchao Xia
@ 2013-12-13 12:19 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2013-12-13 12:19 UTC (permalink / raw)
To: Wenchao Xia; +Cc: armbru, qemu-devel, lcapitulino, imammedo, akong
Il 13/12/2013 03:55, Wenchao Xia ha scritto:
> 于 2013/12/11 2:15, Paolo Bonzini 写道:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 10/12/2013 19:00, Eric Blake ha scritto:
>>>>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
>>>>> + 'gen': 'no' }
>>>
>>> This feels VERY open-coded. No where else in qapi-schema do we
>>> have 'dict' as a type
>>
>> Yes, in fact the "data" field is entirely skipped by the code
>> generator (that's 'gen':'no').
>>
>
> Could we use hmp_object_add()->qmp_object_add()->object_add() code path,
> instead of hmp_object_add()->object_add(),qmp_object_add()->object_add()?
> Would skipping by generator brings some difficult to it?
No, you cannot do that because both hmp_object_add and qmp_object_add
take a QDict. But hmp_object_add's qdict has strings, while
qmp_object_add has the right types (e.g. integer for integer properties).
It is not entirely clear, but actually the structure is the same as
regular commands that use code generation.
The usual code path is
hmp_cont qmp_marshal_input_cont
\ /
qmp_cont
Here it is
hmp_object_add qmp_object_add
\ /
object_add
So the current structure is fine.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-10 18:15 ` Paolo Bonzini
2013-12-13 2:55 ` Wenchao Xia
@ 2013-12-16 20:02 ` Luiz Capitulino
2013-12-17 7:20 ` Markus Armbruster
1 sibling, 1 reply; 22+ messages in thread
From: Luiz Capitulino @ 2013-12-16 20:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: imammedo, akong, qemu-devel, armbru
On Tue, 10 Dec 2013 19:15:05 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 10/12/2013 19:00, Eric Blake ha scritto:
> >>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
> >>> + 'gen': 'no' }
> >
> > This feels VERY open-coded. No where else in qapi-schema do we
> > have 'dict' as a type
>
> Yes, in fact the "data" field is entirely skipped by the code
> generator (that's 'gen':'no').
>
> > ; using it violates all sorts of type-safety (which, I guess, is
> > the point), making it impossible to introspect what keys are valid
> > for use in the "props":{...} dictionary. Do we really want to
> > play this fast and loose with the type system, or should we try
> > harder to make this a robust self-describing union of types?
> >
> > That is, why can't we have object-add use a discriminated union,
> > where qom-type is the discriminator, and where props is an
> > appropriate JSON struct type that corresponds to the branch of the
> > union, so that we get full introspection on the set of valid keys
> > to put in props for any given qom-type?
>
> The point of "props" is passing arbitrary data to a QOM object. We
> should indeed have introspection for QOM objects, where each QOM class
> name can be introspected separately. However, the union of all
> possible QOM objects need not have a "C struct" representation.
The "props" key was added to represent the "O" argument type of
early QMP (which is used by commands like device_add), so that
we could convert them to the QAPI. IIRC, we didn't plan for it
to be used by new commands... But I don't have anything better
to suggest, so I won't object to its usage here.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
` (5 preceding siblings ...)
2013-12-12 15:43 ` [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Igor Mammedov
@ 2013-12-16 20:03 ` Luiz Capitulino
6 siblings, 0 replies; 22+ messages in thread
From: Luiz Capitulino @ 2013-12-16 20:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: imammedo, akong, qemu-devel, armbru
On Tue, 10 Dec 2013 18:00:23 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> These allow hotplugging (and hot-unplugging without leaking an object)
> virtio-rng devices. They can also be used for memory hotplug.
Looks OK to me.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-16 20:02 ` Luiz Capitulino
@ 2013-12-17 7:20 ` Markus Armbruster
2013-12-19 21:53 ` Michael Roth
0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2013-12-17 7:20 UTC (permalink / raw)
To: Luiz Capitulino
Cc: Michael Roth, qemu-devel, Anthony Liguori, imammedo,
Paolo Bonzini, akong
[Cc: Anthony, Mike for QAPI schema expertise]
Luiz Capitulino <lcapitulino@redhat.com> writes:
> On Tue, 10 Dec 2013 19:15:05 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 10/12/2013 19:00, Eric Blake ha scritto:
>> >>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
>> >>> + 'gen': 'no' }
>> >
>> > This feels VERY open-coded. No where else in qapi-schema do we
>> > have 'dict' as a type
>>
>> Yes, in fact the "data" field is entirely skipped by the code
>> generator (that's 'gen':'no').
>>
>> > ; using it violates all sorts of type-safety (which, I guess, is
>> > the point), making it impossible to introspect what keys are valid
>> > for use in the "props":{...} dictionary. Do we really want to
>> > play this fast and loose with the type system, or should we try
>> > harder to make this a robust self-describing union of types?
>> >
>> > That is, why can't we have object-add use a discriminated union,
>> > where qom-type is the discriminator, and where props is an
>> > appropriate JSON struct type that corresponds to the branch of the
>> > union, so that we get full introspection on the set of valid keys
>> > to put in props for any given qom-type?
>>
>> The point of "props" is passing arbitrary data to a QOM object. We
>> should indeed have introspection for QOM objects, where each QOM class
>> name can be introspected separately. However, the union of all
>> possible QOM objects need not have a "C struct" representation.
>
> The "props" key was added to represent the "O" argument type of
> early QMP (which is used by commands like device_add), so that
> we could convert them to the QAPI. IIRC, we didn't plan for it
> to be used by new commands... But I don't have anything better
> to suggest, so I won't object to its usage here.
We created monitor argument type "O" to have name=val,... arguments in
the human monitor exactly like command line option arguments. Currently
used by device_add and netdev_add.
We shoehorned type "O" into QMP in a bout of QMP feature-completeness
desperation. This was before QAPI.
device_add still isn't in qapi-schema.json, but netdev_add is:
{ 'command': 'netdev_add',
'data': {'type': 'str', 'id': 'str', '*props': '**'},
'gen': 'no' }
Note the magic "'*props': '**'" (I'll be hanged if I know what that
means[*]), and "'gen': 'no'".
Yes, a proper schema for netdev_add and device_add is desirable. In
both cases (but especially for device_add), the arguments are the
obligatory id plus a union discriminated by the device type, contraining
that device's properties.
Unless we move device properties definition to qapi-schema.json (bad),
or duplicate them there (worse), we need to derive that part of the
schema dynamically from device information available in QOM.
[*] Can we have a definition of QAPI schema semantics other than code?
Pretty-please?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-17 7:20 ` Markus Armbruster
@ 2013-12-19 21:53 ` Michael Roth
2014-01-07 12:00 ` Markus Armbruster
0 siblings, 1 reply; 22+ messages in thread
From: Michael Roth @ 2013-12-19 21:53 UTC (permalink / raw)
To: Markus Armbruster, Luiz Capitulino
Cc: qemu-devel, Anthony Liguori, imammedo, Paolo Bonzini, akong
Quoting Markus Armbruster (2013-12-17 01:20:16)
> [Cc: Anthony, Mike for QAPI schema expertise]
>
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> > On Tue, 10 Dec 2013 19:15:05 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Il 10/12/2013 19:00, Eric Blake ha scritto:
> >> >>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
> >> >>> + 'gen': 'no' }
> >> >
> >> > This feels VERY open-coded. No where else in qapi-schema do we
> >> > have 'dict' as a type
> >>
> >> Yes, in fact the "data" field is entirely skipped by the code
> >> generator (that's 'gen':'no').
> >>
> >> > ; using it violates all sorts of type-safety (which, I guess, is
> >> > the point), making it impossible to introspect what keys are valid
> >> > for use in the "props":{...} dictionary. Do we really want to
> >> > play this fast and loose with the type system, or should we try
> >> > harder to make this a robust self-describing union of types?
> >> >
> >> > That is, why can't we have object-add use a discriminated union,
> >> > where qom-type is the discriminator, and where props is an
> >> > appropriate JSON struct type that corresponds to the branch of the
> >> > union, so that we get full introspection on the set of valid keys
> >> > to put in props for any given qom-type?
> >>
> >> The point of "props" is passing arbitrary data to a QOM object. We
> >> should indeed have introspection for QOM objects, where each QOM class
> >> name can be introspected separately. However, the union of all
> >> possible QOM objects need not have a "C struct" representation.
> >
> > The "props" key was added to represent the "O" argument type of
> > early QMP (which is used by commands like device_add), so that
> > we could convert them to the QAPI. IIRC, we didn't plan for it
> > to be used by new commands... But I don't have anything better
> > to suggest, so I won't object to its usage here.
>
> We created monitor argument type "O" to have name=val,... arguments in
> the human monitor exactly like command line option arguments. Currently
> used by device_add and netdev_add.
>
> We shoehorned type "O" into QMP in a bout of QMP feature-completeness
> desperation. This was before QAPI.
>
> device_add still isn't in qapi-schema.json, but netdev_add is:
>
> { 'command': 'netdev_add',
> 'data': {'type': 'str', 'id': 'str', '*props': '**'},
> 'gen': 'no' }
>
> Note the magic "'*props': '**'" (I'll be hanged if I know what that
> means[*]), and "'gen': 'no'".
>
> Yes, a proper schema for netdev_add and device_add is desirable. In
> both cases (but especially for device_add), the arguments are the
> obligatory id plus a union discriminated by the device type, contraining
> that device's properties.
>
> Unless we move device properties definition to qapi-schema.json (bad),
> or duplicate them there (worse), we need to derive that part of the
> schema dynamically from device information available in QOM.
Is dumping static properties based on class name sufficient, or do we
need introspection for dynamic properties as well? (or are those not
exposed outside of qom-set?) We could maybe introduce a QAPI 'built-in'
such as 'ObjectProperties' that automatically does the query based on the
now-special 'type' param and handles all the type-checking up-front. This
would avoid an open-ended 'dict' type proliferating too much and provide
infrastructure for introspection.
>
>
> [*] Can we have a definition of QAPI schema semantics other than code?
> Pretty-please?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2013-12-19 21:53 ` Michael Roth
@ 2014-01-07 12:00 ` Markus Armbruster
2014-01-07 12:51 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2014-01-07 12:00 UTC (permalink / raw)
To: Michael Roth
Cc: qemu-devel, Luiz Capitulino, Anthony Liguori, Paolo Bonzini,
imammedo, akong
Michael Roth <mdroth@linux.vnet.ibm.com> writes:
> Quoting Markus Armbruster (2013-12-17 01:20:16)
>> [Cc: Anthony, Mike for QAPI schema expertise]
>>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>
>> > On Tue, 10 Dec 2013 19:15:05 +0100
>> > Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >
>> >> -----BEGIN PGP SIGNED MESSAGE-----
>> >> Hash: SHA1
>> >>
>> >> Il 10/12/2013 19:00, Eric Blake ha scritto:
>> >> >>> + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
>> >> >>> + 'gen': 'no' }
>> >> >
>> >> > This feels VERY open-coded. No where else in qapi-schema do we
>> >> > have 'dict' as a type
>> >>
>> >> Yes, in fact the "data" field is entirely skipped by the code
>> >> generator (that's 'gen':'no').
>> >>
>> >> > ; using it violates all sorts of type-safety (which, I guess, is
>> >> > the point), making it impossible to introspect what keys are valid
>> >> > for use in the "props":{...} dictionary. Do we really want to
>> >> > play this fast and loose with the type system, or should we try
>> >> > harder to make this a robust self-describing union of types?
>> >> >
>> >> > That is, why can't we have object-add use a discriminated union,
>> >> > where qom-type is the discriminator, and where props is an
>> >> > appropriate JSON struct type that corresponds to the branch of the
>> >> > union, so that we get full introspection on the set of valid keys
>> >> > to put in props for any given qom-type?
>> >>
>> >> The point of "props" is passing arbitrary data to a QOM object. We
>> >> should indeed have introspection for QOM objects, where each QOM class
>> >> name can be introspected separately. However, the union of all
>> >> possible QOM objects need not have a "C struct" representation.
>> >
>> > The "props" key was added to represent the "O" argument type of
>> > early QMP (which is used by commands like device_add), so that
>> > we could convert them to the QAPI. IIRC, we didn't plan for it
>> > to be used by new commands... But I don't have anything better
>> > to suggest, so I won't object to its usage here.
>>
>> We created monitor argument type "O" to have name=val,... arguments in
>> the human monitor exactly like command line option arguments. Currently
>> used by device_add and netdev_add.
>>
>> We shoehorned type "O" into QMP in a bout of QMP feature-completeness
>> desperation. This was before QAPI.
>>
>> device_add still isn't in qapi-schema.json, but netdev_add is:
>>
>> { 'command': 'netdev_add',
>> 'data': {'type': 'str', 'id': 'str', '*props': '**'},
>> 'gen': 'no' }
>>
>> Note the magic "'*props': '**'" (I'll be hanged if I know what that
>> means[*]), and "'gen': 'no'".
>>
>> Yes, a proper schema for netdev_add and device_add is desirable. In
>> both cases (but especially for device_add), the arguments are the
>> obligatory id plus a union discriminated by the device type, contraining
>> that device's properties.
>>
>> Unless we move device properties definition to qapi-schema.json (bad),
>> or duplicate them there (worse), we need to derive that part of the
>> schema dynamically from device information available in QOM.
>
> Is dumping static properties based on class name sufficient, or do we
> need introspection for dynamic properties as well? (or are those not
> exposed outside of qom-set?)
Can't say whether introspection limited to static properties would be
useful, because I don't actually know how static and dynamic properties
are used.
I challenged the need for dynamic properties in the early QOM days,
because they lead to dynamic schemata and complicate introspection, but
Anthony assured us we cannot do without them.
> We could maybe introduce a QAPI 'built-in'
> such as 'ObjectProperties' that automatically does the query based on the
> now-special 'type' param and handles all the type-checking up-front. This
> would avoid an open-ended 'dict' type proliferating too much and provide
> infrastructure for introspection.
I'm afraid I'm too ignorant to understand what you're proposing.
>> [*] Can we have a definition of QAPI schema semantics other than code?
>> Pretty-please?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
2014-01-07 12:00 ` Markus Armbruster
@ 2014-01-07 12:51 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2014-01-07 12:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Michael Roth, Luiz Capitulino, Anthony Liguori,
imammedo, akong
Il 07/01/2014 13:00, Markus Armbruster ha scritto:
>> >
>> > Is dumping static properties based on class name sufficient, or do we
>> > need introspection for dynamic properties as well? (or are those not
>> > exposed outside of qom-set?)
>
> Can't say whether introspection limited to static properties would be
> useful, because I don't actually know how static and dynamic properties
> are used.
I think right now dynamic properties are used little-to-nothing.
They are used by non-devices such as the RNG backends, and for links
(which should replace PROP_PTR).
> I challenged the need for dynamic properties in the early QOM days,
> because they lead to dynamic schemata and complicate introspection, but
> Anthony assured us we cannot do without them.
Dynamic properties are the basis of the QOM tree *as it is implemented
now*. Children are simply dynamic properties of type child<SomeType>,
and they must be dynamic since the command-line influences the QOM tree.
So I agree with Anthony that dynamic properties are fundamental to the
way QOM is implemented. I'm not sure it is *necessary* for it to be
implemented that way, but I don't have any code to show that another
implementation would be simpler or better or even possible.
I also agree with him that moving the qdev properties code up to Object
is not the right way to proceed in order to have static properties.
That code has support for legacy property types such as hexadecimal,
that we need not have outside -device (i.e. if we can create devices one
day with -object, you'll have to write 0x402 because 402 will always be
interpreted as decimal). At the same time it does not support neither
links nor the more flexible getters/setters that QOM allows. We
probably would want this for QOM static properties.
On the other hand I agree with a large subset of your idea, i.e. that
devices should have a known schema for the subset of properties that can
be set at initialization time. Furthermore, this schema should be
introspectable.
Anyhow, with so little use of dynamic properties (outside children), I
believe no one can argue his position successfully with code rather than
just theories.
> > We could maybe introduce a QAPI 'built-in'
> > such as 'ObjectProperties' that automatically does the query based on the
> > now-special 'type' param and handles all the type-checking up-front. This
> > would avoid an open-ended 'dict' type proliferating too much and provide
> > infrastructure for introspection.
>
> I'm afraid I'm too ignorant to understand what you're proposing.
I don't really understand that either.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-01-07 12:52 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
2013-12-11 23:14 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object Paolo Bonzini
2013-12-11 23:15 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child Paolo Bonzini
2013-12-11 23:16 ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command Paolo Bonzini
2013-12-10 18:00 ` Eric Blake
2013-12-10 18:15 ` Paolo Bonzini
2013-12-13 2:55 ` Wenchao Xia
2013-12-13 12:19 ` Paolo Bonzini
2013-12-16 20:02 ` Luiz Capitulino
2013-12-17 7:20 ` Markus Armbruster
2013-12-19 21:53 ` Michael Roth
2014-01-07 12:00 ` Markus Armbruster
2014-01-07 12:51 ` Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del " Paolo Bonzini
2013-12-10 18:01 ` Eric Blake
2013-12-10 18:17 ` Paolo Bonzini
2013-12-12 15:43 ` [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Igor Mammedov
2013-12-16 20:03 ` Luiz Capitulino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).