Copying Andreas and Paolo for QOM expertise. "Daniel P. Berrange" <berra...@redhat.com> writes:
> Currently both object_del and device_del require that the > client provide the object/device short ID. While user > creatable objects require an ID to be provided at time of > creation, qdev devices may be created without giving an > ID. The only unique identifier they would then have is the > QOM object path. > > Allowing device_del to accept an object path ensures all > devices are deletable regardless of whether they have an > ID. > > (qemu) device_add usb-mouse > (qemu) qom-list /machine/peripheral-anon > device[0] (child<usb-mouse>) > type (string) > (qemu) device_del /machine/peripheral-anon/device[0] > > Although objects require an ID to be provided upfront, > there may be cases where the client would prefer to > use QOM paths when deleting. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> I believe this makes sense no matter what we do about device IDs (see thread "Should we auto-generate IDs?"). > --- > hmp-commands.hx | 6 ++++-- > qdev-monitor.c | 14 +++++++++----- > qmp-commands.hx | 17 +++++++++++++---- > qmp.c | 10 +++++++--- > 4 files changed, 33 insertions(+), 14 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index d3b7932..c0c113d 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -675,7 +675,8 @@ ETEXI > STEXI > @item device_del @var{id} > @findex device_del > -Remove device @var{id}. > +Remove device @var{id}. @var{id} may be a short ID > +or a QOM object path. > ETEXI > > { > @@ -1279,7 +1280,8 @@ ETEXI > STEXI > @item object_del > @findex object_del > -Destroy QOM object. > +Destroy QOM object. @var{id} may be a short ID > +or a QOM object path. > ETEXI > > #ifdef CONFIG_SLIRP > diff --git a/qdev-monitor.c b/qdev-monitor.c > index f9e2d62..894b799 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -785,13 +785,17 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, > Error **errp) > void qmp_device_del(const char *id, Error **errp) > { > Object *obj; > - char *root_path = object_get_canonical_path(qdev_get_peripheral()); > - char *path = g_strdup_printf("%s/%s", root_path, id); > > - g_free(root_path); > - obj = object_resolve_path_type(path, TYPE_DEVICE, NULL); > - g_free(path); > + if (id[0] == '/') { > + obj = object_resolve_path(id, NULL); > + } else { > + char *root_path = object_get_canonical_path(qdev_get_peripheral()); > + char *path = g_strdup_printf("%s/%s", root_path, id); > > + g_free(root_path); > + obj = object_resolve_path_type(path, TYPE_DEVICE, NULL); > + g_free(path); > + } > if (!obj) { > error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > "Device '%s' not found", id); Easier to review with whitespace change ignored: diff --git a/qdev-monitor.c b/qdev-monitor.c index f9e2d62..894b799 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -785,13 +785,17 @@ void qmp_device_del(const char *id, Error **errp) { Object *obj; + + if (id[0] == '/') { + obj = object_resolve_path(id, NULL); + } else { char *root_path = object_get_canonical_path(qdev_get_peripheral()); char *path = g_strdup_printf("%s/%s", root_path, id); g_free(root_path); obj = object_resolve_path_type(path, TYPE_DEVICE, NULL); g_free(path); - + } if (!obj) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", id); I'd keep the blank line between the code finding obj and the if (!obj) error check. @id is a suboptimal name now, but changing it isn't worth the churn. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ba630b1..d3070cf 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -321,14 +321,19 @@ Remove a device. > > Arguments: > > -- "id": the device's ID (json-string) > - > +- "id": the device's ID or QOM path (json-string) > +EQ > Example: > > -> { "execute": "device_del", "arguments": { "id": "net1" } } > <- { "return": {} } > > -EQMP > +Example: > + > +-> { "execute": "device_del", "arguments": { "id": > "/machine/peripheral-anon/device[0]" } } > +<- { "return": {} } > + > +MP > > { > .name = "send-key", > @@ -965,7 +970,7 @@ Remove QOM object. > > Arguments: > > -- "id": the object's ID (json-string) > +- "id": the object's ID or QOM path (json-string) > > Example: > > @@ -973,6 +978,10 @@ Example: > <- { "return": {} } > > > +-> { "execute": "object-del", "arguments": { "id": "/objects/hostmem1" } } > +<- { "return": {} } > + > + > EQMP > > Thanks for adding examples. As Eric said, you should also update qapi-schema.json. The duplication is unfortunate. There's an RFC PATCH series from Marc-André pending review, which should get us one step closer to killing qmp-commands.hx. > diff --git a/qmp.c b/qmp.c > index 403805a..11e9f51 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -678,11 +678,15 @@ void qmp_object_add(QDict *qdict, QObject **ret, Error > **errp) > > 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 (id[0] == '/') { > + obj = object_resolve_path(id, NULL); > + } else { > + Object *container; > + container = object_get_objects_root(); > + obj = object_resolve_path_component(container, id); > + } > if (!obj) { > error_setg(errp, "object id not found"); > return; Ignoring whitespace change: diff --git a/qmp.c b/qmp.c index 403805a..11e9f51 100644 --- a/qmp.c +++ b/qmp.c @@ -678,11 +678,15 @@ void qmp_object_del(const char *id, Error **errp) { - Object *container; Object *obj; + if (id[0] == '/') { + obj = object_resolve_path(id, NULL); + } else { + Object *container; container = object_get_objects_root(); obj = object_resolve_path_component(container, id); + } if (!obj) { error_setg(errp, "object id not found"); return; I'd keep container right where it is. We generally prefer keeping declarations at the beginning of the function body, at least for small functions. Update qapi-schema.json updated the obvious way, and you can have my R-by. Also addressing my stylistic nitpicks would be nice. Thanks for tackling this, it's long overdue!