| Message ID | 1387279564-517-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 17, 2013 at 9:26 PM, Paolo Bonzini <pbonzini@redhat.com> 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> > --- > 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; > + } So I have been thinking about repeated if(error_is_set(&err)) { goto foo; } and how to reduce its verbosity in situations like this. Can it be solved with a simple semantic: "Error ** accepting APIs will perform no action if the Error ** argument is already set." In this case we would patch visit_foo to just return if error_is_set(errp). Then we can delete these three LOC... > + > + qdict_del(pdict, "qom-type"); > + visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err); > + if (error_is_set(&err)) { > + goto out_clean; > + } > + and these > + qdict_del(pdict, "id"); > + visit_type_str(opts_get_visitor(ov), &id, "id", &err); > + if (error_is_set(&err)) { > + goto out_clean; > + } and these > + > + object_add(type, id, pdict, opts_get_visitor(ov), &err); > + if (error_is_set(&err)) { > + goto out_clean; > + } And leave just the last one which will catch and report the first-occured error as desired. Out of scope of the series of course. Regards, Peter > + 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 dictionary 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 > > >
Il 17/12/2013 12:54, Peter Crosthwaite ha scritto: >> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err); >> + if (error_is_set(&err)) { >> + goto out_clean; >> + } > > So I have been thinking about repeated if(error_is_set(&err)) { goto > foo; } and how to reduce its verbosity in situations like this. Can it > be solved with a simple semantic: > > "Error ** accepting APIs will perform no action if the Error ** > argument is already set." I think this is a case where verbosity <<< ease of use. In this case, the caller code is particularly simple, but what if I needed to dereference the return value of the first called function, to get the argument to the second? You would still need an "if". Paolo > In this case we would patch visit_foo to just return if > error_is_set(errp). Then we can delete these three LOC... > >> + >> + qdict_del(pdict, "qom-type"); >> + visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err); >> + if (error_is_set(&err)) { >> + goto out_clean; >> + } >> + > > and these > >> + qdict_del(pdict, "id"); >> + visit_type_str(opts_get_visitor(ov), &id, "id", &err); >> + if (error_is_set(&err)) { >> + goto out_clean; >> + } > > and these > >> + >> + object_add(type, id, pdict, opts_get_visitor(ov), &err); >> + if (error_is_set(&err)) { >> + goto out_clean; >> + } > > And leave just the last one which will catch and report the > first-occured error as desired. > > Out of scope of the series of course. > > Regards, > Peter > >> + 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 dictionary 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 >> >> >>
On Tue, Dec 17, 2013 at 10:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 17/12/2013 12:54, Peter Crosthwaite ha scritto: >>> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err); >>> + if (error_is_set(&err)) { >>> + goto out_clean; >>> + } >> >> So I have been thinking about repeated if(error_is_set(&err)) { goto >> foo; } and how to reduce its verbosity in situations like this. Can it >> be solved with a simple semantic: >> >> "Error ** accepting APIs will perform no action if the Error ** >> argument is already set." > > I think this is a case where verbosity <<< ease of use. > > In this case, the caller code is particularly simple, but what if I > needed to dereference the return value of the first called function, to > get the argument to the second? You would still need an "if". > Yes thats right. This isn't going to work universally and callers will always have the responsibility of knowing whether they can continue or not. But it will help a lot for repetitive collections of similar independent functions calls. The ultimate example is probably the device tree API calls in hw/ppc/e500.c. I want to patch the device tree API to be nice and Error**ified (for my own future reasons) but I sure don't want to have to patch e500 to check every qemu_devtree_foo API call with these 3 LOC. TBH i'll probably just preserve current behavior using &error_abort in next rev of that series, but it should be possible to do less verbose caller-customized collective error handling in some way. Regards, Peter
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > On Tue, Dec 17, 2013 at 10:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 17/12/2013 12:54, Peter Crosthwaite ha scritto: >>>> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err); >>>> + if (error_is_set(&err)) { >>>> + goto out_clean; >>>> + } >>> >>> So I have been thinking about repeated if(error_is_set(&err)) { goto >>> foo; } and how to reduce its verbosity in situations like this. Can it >>> be solved with a simple semantic: >>> >>> "Error ** accepting APIs will perform no action if the Error ** >>> argument is already set." >> >> I think this is a case where verbosity <<< ease of use. >> >> In this case, the caller code is particularly simple, but what if I >> needed to dereference the return value of the first called function, to >> get the argument to the second? You would still need an "if". >> > > Yes thats right. This isn't going to work universally and callers will > always have the responsibility of knowing whether they can continue or > not. But it will help a lot for repetitive collections of similar > independent functions calls. The ultimate example is probably the > device tree API calls in hw/ppc/e500.c. > > I want to patch the device tree API to be nice and Error**ified (for > my own future reasons) but I sure don't want to have to patch e500 to > check every qemu_devtree_foo API call with these 3 LOC. TBH i'll > probably just preserve current behavior using &error_abort in next rev > of that series, but it should be possible to do less verbose > caller-customized collective error handling in some way. error_set() & friends currently have a "errp doesn't contain an error already" precondition: if (errp == NULL) { return; } assert(*errp == NULL); You could change the function contracts to ignore additional errors. Theoretical drawback: in situations where that isn't intended, programming errors no longer get caught. If people actually care for that enough to veto your change, you can try adding new functions for use when it is intended.
On Tue, Dec 17, 2013 at 11:07 PM, Markus Armbruster <armbru@redhat.com> wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> On Tue, Dec 17, 2013 at 10:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 17/12/2013 12:54, Peter Crosthwaite ha scritto: >>>>> + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err); >>>>> + if (error_is_set(&err)) { >>>>> + goto out_clean; >>>>> + } >>>> >>>> So I have been thinking about repeated if(error_is_set(&err)) { goto >>>> foo; } and how to reduce its verbosity in situations like this. Can it >>>> be solved with a simple semantic: >>>> >>>> "Error ** accepting APIs will perform no action if the Error ** >>>> argument is already set." >>> >>> I think this is a case where verbosity <<< ease of use. >>> >>> In this case, the caller code is particularly simple, but what if I >>> needed to dereference the return value of the first called function, to >>> get the argument to the second? You would still need an "if". >>> >> >> Yes thats right. This isn't going to work universally and callers will >> always have the responsibility of knowing whether they can continue or >> not. But it will help a lot for repetitive collections of similar >> independent functions calls. The ultimate example is probably the >> device tree API calls in hw/ppc/e500.c. >> >> I want to patch the device tree API to be nice and Error**ified (for >> my own future reasons) but I sure don't want to have to patch e500 to >> check every qemu_devtree_foo API call with these 3 LOC. TBH i'll >> probably just preserve current behavior using &error_abort in next rev >> of that series, but it should be possible to do less verbose >> caller-customized collective error handling in some way. > > error_set() & friends currently have a "errp doesn't contain an error > already" precondition: > > if (errp == NULL) { > return; > } > assert(*errp == NULL); > > You could change the function contracts to ignore additional errors. > Theoretical drawback: in situations where that isn't intended, > programming errors no longer get caught. If people actually care for > that enough to veto your change, you can try adding new functions for > use when it is intended. > That will work for some cases. But pre-checking in the API call ASAP guards you against the case where subsequent API calls depends upon the first. This is similar to the problem Paolo flagged. What happens if an API returns a ptr but fails, then that ptr is passes to the second API which derefs it? (before any error checks). Regards, Peter
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 dictionary 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; +}
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(-)