| Message ID | 1440689864-32127-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/27/2015 09:37 AM, Daniel P. Berrange wrote: > 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 <berrange@redhat.com> > --- > hmp-commands.hx | 6 ++++-- > qdev-monitor.c | 14 +++++++++----- > qmp-commands.hx | 17 +++++++++++++---- > qmp.c | 10 +++++++--- > 4 files changed, 33 insertions(+), 14 deletions(-) Might also want to touch qapi-schema.json for consistent documentation of the @id parameter (our goal is for qmp-commands.hx to go away some day; and while we can merge contents at that time, it's nicer to keep them in sync now) Reviewed-by: Eric Blake <eblake@redhat.com
On 2015/8/27 23:37, Daniel P. Berrange wrote: > 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 <berrange@redhat.com> > --- > hmp-commands.hx | 6 ++++-- > qdev-monitor.c | 14 +++++++++----- > qmp-commands.hx | 17 +++++++++++++---- > qmp.c | 10 +++++++--- > 4 files changed, 33 insertions(+), 14 deletions(-) > It's useful. Reviewed-by: Gonglei <arei.gonglei@huawei.com> > 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); > 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 > > > 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; >
Copying Andreas and Paolo for QOM expertise. "Daniel P. Berrange" <berrange@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 <berrange@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!
On Fri, Aug 28, 2015 at 02:53:41PM +0200, Markus Armbruster wrote: > Copying Andreas and Paolo for QOM expertise. > > "Daniel P. Berrange" <berrange@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 <berrange@redhat.com> > > I believe this makes sense no matter what we do about device IDs (see > thread "Should we auto-generate IDs?"). [snip] > Update qapi-schema.json updated the obvious way, and you can have my > R-by. Also addressing my stylistic nitpicks would be nice. Already posted a v2 with the qapi-schema.json addition after Eric pointed it out :-) Regards, Daniel
On 28/08/2015 14:53, Markus Armbruster wrote: > I believe this makes sense no matter what we do about device IDs (see > thread "Should we auto-generate IDs?"). I haven't read that huge thread yet, but I think it gives the user too much power. There are internal objects that are not supposed to be freed, and freeing them would likely result in a SEGV or similar. Paolo
On 2015/8/29 14:49, Paolo Bonzini wrote: > > > On 28/08/2015 14:53, Markus Armbruster wrote: >> I believe this makes sense no matter what we do about device IDs (see >> thread "Should we auto-generate IDs?"). > > I haven't read that huge thread yet, but I think it gives the user too > much power. There are internal objects that are not supposed to be > freed, and freeing them would likely result in a SEGV or similar. > If one object do not support hot-unplug, the interface will return simply with error messages. ;) Regards, -Gonglei
On Sat, Aug 29, 2015 at 08:49:58AM +0200, Paolo Bonzini wrote: > > > On 28/08/2015 14:53, Markus Armbruster wrote: > > I believe this makes sense no matter what we do about device IDs (see > > thread "Should we auto-generate IDs?"). > > I haven't read that huge thread yet, but I think it gives the user too > much power. There are internal objects that are not supposed to be > freed, and freeing them would likely result in a SEGV or similar. I'll double check, but I thought it raised a nice error if the object did not implement the user-creatable interface. Regards, Daniel
On Tue, Sep 01, 2015 at 09:58:14AM +0100, Daniel P. Berrange wrote: > On Sat, Aug 29, 2015 at 08:49:58AM +0200, Paolo Bonzini wrote: > > > > > > On 28/08/2015 14:53, Markus Armbruster wrote: > > > I believe this makes sense no matter what we do about device IDs (see > > > thread "Should we auto-generate IDs?"). > > > > I haven't read that huge thread yet, but I think it gives the user too > > much power. There are internal objects that are not supposed to be > > freed, and freeing them would likely result in a SEGV or similar. > > I'll double check, but I thought it raised a nice error if the object > did not implement the user-creatable interface. Ok, I'm wrong - we get an assertion error because it isn't user-creatble and then we abort. I'll send a v3 which returns a pretty error instead of aborting. Regards, Daniel
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); 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 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;
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 <berrange@redhat.com> --- hmp-commands.hx | 6 ++++-- qdev-monitor.c | 14 +++++++++----- qmp-commands.hx | 17 +++++++++++++---- qmp.c | 10 +++++++--- 4 files changed, 33 insertions(+), 14 deletions(-)