| Message ID | 1441101059-14269-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015/9/1 17:50, 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. > > Devices are required to be marked as hotpluggable > otherwise an error is raised > > (qemu) device_del /machine/unattached/device[4] > Device 'PIIX3' does not support hotplugging > > Similarly objects are required to implement the > user-creatable interface > > (qemu) object_del /machine/unattached/device[4] > /machine/unattached/device[4] is not a user-creatable object > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > > Changed in v3: > > - Add type checks to avoid assertion failures if user > supplied path is not of type device or user-creatable > > hmp-commands.hx | 6 ++++-- > qapi-schema.json | 4 ++-- > qdev-monitor.c | 19 ++++++++++++++----- > qmp-commands.hx | 13 +++++++++++-- > qmp.c | 15 ++++++++++++--- > 5 files changed, 43 insertions(+), 14 deletions(-) Reviewed-by: Gonglei <arei.gonglei@huawei.com> Tested-by: Gonglei <arei.gonglei@huawei.com>
Am 01.09.2015 um 11:50 schrieb Daniel P. Berrange: > 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. > > Devices are required to be marked as hotpluggable > otherwise an error is raised > > (qemu) device_del /machine/unattached/device[4] > Device 'PIIX3' does not support hotplugging > > Similarly objects are required to implement the > user-creatable interface > > (qemu) object_del /machine/unattached/device[4] > /machine/unattached/device[4] is not a user-creatable object > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > > Changed in v3: > > - Add type checks to avoid assertion failures if user > supplied path is not of type device or user-creatable > > hmp-commands.hx | 6 ++++-- > qapi-schema.json | 4 ++-- > qdev-monitor.c | 19 ++++++++++++++----- > qmp-commands.hx | 13 +++++++++++-- > qmp.c | 15 ++++++++++++--- > 5 files changed, 43 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. Have you considered using two alternative parameters, id and qom-path? (qom_path was used elsewhere) Regards, Andreas > 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 [snip]
On Tue, Sep 01, 2015 at 03:17:29PM +0200, Andreas Färber wrote: > Am 01.09.2015 um 11:50 schrieb Daniel P. Berrange: > > 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. > > > > Devices are required to be marked as hotpluggable > > otherwise an error is raised > > > > (qemu) device_del /machine/unattached/device[4] > > Device 'PIIX3' does not support hotplugging > > > > Similarly objects are required to implement the > > user-creatable interface > > > > (qemu) object_del /machine/unattached/device[4] > > /machine/unattached/device[4] is not a user-creatable object > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > > > Changed in v3: > > > > - Add type checks to avoid assertion failures if user > > supplied path is not of type device or user-creatable > > > > hmp-commands.hx | 6 ++++-- > > qapi-schema.json | 4 ++-- > > qdev-monitor.c | 19 ++++++++++++++----- > > qmp-commands.hx | 13 +++++++++++-- > > qmp.c | 15 ++++++++++++--- > > 5 files changed, 43 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. > > Have you considered using two alternative parameters, id and qom-path? > (qom_path was used elsewhere) I'm not fussed either way, but I thought it simpler to not try to change the accepted parameters of the existing commands. Looking, the only place I notice that uses a 'qom_path' is the return data in the CpuInfo struct. Does anyone have strong feelings either way about use of id for both vs qom-path or id ? Regards, Daniel
On 09/01/2015 07:23 AM, Daniel P. Berrange wrote: >>> +Remove device @var{id}. @var{id} may be a short ID >>> +or a QOM object path. >> >> Have you considered using two alternative parameters, id and qom-path? >> (qom_path was used elsewhere) > > I'm not fussed either way, but I thought it simpler to not try to change > the accepted parameters of the existing commands. Looking, the only > place I notice that uses a 'qom_path' is the return data in the CpuInfo > struct. > > Does anyone have strong feelings either way about use of id for both vs > qom-path or id ? Reusing 'id': - Pros - less complicated interface (don't have to check for mutual exclusion) - Cons - not introspectible (can't tell by introspection alone whether id can take a QOM path) - confusing name (but not the first time we've had that issue) Adding 'qom-path': - Pros - introspectible - JSON expresses everything (we don't have to parse the first character of the string to know which style was meant, as the choice of key already decided it) - Cons - Have to implement mutual exclusion ourselves (can't take 'id' and 'qom-path' at the same time, and at least one must be specified), unless we invent a new way for qapi to express mutual exclusion (there are other existing commands that would benefit from such an extension) How likely is libvirt to need to introspect whether this extension is available? We've already argued that libvirt should already be giving everything an id, and therefore, the existing semantics of 'id' are already sufficient for libvirt's needs, and libvirt will never need to delete by qom-path, and thus does not need to introspect whether qom-path even works. Therefore, I'm leaning towards simplicity rather than introspection; and the patch is fine as-is from my viewpoint.
On 09/01/2015 03:50 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. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > > Changed in v3: > > - Add type checks to avoid assertion failures if user > supplied path is not of type device or user-creatable > Reviewed-by: Eric Blake <eblake@redhat.com> Pre-existing, but may want to fix: > +++ b/qmp.c > @@ -678,16 +678,25 @@ void qmp_object_add(QDict *qdict, QObject **ret, Error **errp) > > void qmp_object_del(const char *id, Error **errp) > + if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { > + error_setg(errp, "%s is not a user-creatable object", id); > + return; > + } > + > if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) { > error_setg(errp, "%s is in use, can not be deleted", id); s/can not/cannot/
On Sep 1, 2015, at 11:55 AM, Eric Blake wrote: > On 09/01/2015 07:23 AM, Daniel P. Berrange wrote: > >>>> +Remove device @var{id}. @var{id} may be a short ID >>>> +or a QOM object path. >>> >>> Have you considered using two alternative parameters, id and qom-path? >>> (qom_path was used elsewhere) >> >> I'm not fussed either way, but I thought it simpler to not try to change >> the accepted parameters of the existing commands. Looking, the only >> place I notice that uses a 'qom_path' is the return data in the CpuInfo >> struct. >> >> Does anyone have strong feelings either way about use of id for both vs >> qom-path or id ? > > Reusing 'id': > - Pros > - less complicated interface (don't have to check for mutual exclusion) > - Cons > - not introspectible (can't tell by introspection alone whether id can > take a QOM path) > - confusing name (but not the first time we've had that issue) > > Adding 'qom-path': > - Pros > - introspectible > - JSON expresses everything (we don't have to parse the first > character of the string to know which style was meant, as the choice of > key already decided it) > - Cons > - Have to implement mutual exclusion ourselves (can't take 'id' and > 'qom-path' at the same time, and at least one must be specified), unless > we invent a new way for qapi to express mutual exclusion (there are > other existing commands that would benefit from such an extension) Don't forget having a really long command to type up just to find out what qom path you need. Also the qom path itself is very long. A simple ID is much easier to type out. > > How likely is libvirt to need to introspect whether this extension is > available? We've already argued that libvirt should already be giving > everything an id, and therefore, the existing semantics of 'id' are > already sufficient for libvirt's needs, and libvirt will never need to > delete by qom-path, and thus does not need to introspect whether > qom-path even works. > > Therefore, I'm leaning towards simplicity rather than introspection; and > the patch is fine as-is from my viewpoint. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Am 01.09.2015 um 17:58 schrieb Programmingkid: > On Sep 1, 2015, at 11:55 AM, Eric Blake wrote: >> On 09/01/2015 07:23 AM, Daniel P. Berrange wrote: >>>>> +Remove device @var{id}. @var{id} may be a short ID >>>>> +or a QOM object path. >>>> >>>> Have you considered using two alternative parameters, id and qom-path? >>>> (qom_path was used elsewhere) >>> >>> I'm not fussed either way, but I thought it simpler to not try to change >>> the accepted parameters of the existing commands. Looking, the only >>> place I notice that uses a 'qom_path' is the return data in the CpuInfo >>> struct. >>> >>> Does anyone have strong feelings either way about use of id for both vs >>> qom-path or id ? >> >> Reusing 'id': >> - Pros >> - less complicated interface (don't have to check for mutual exclusion) >> - Cons >> - not introspectible (can't tell by introspection alone whether id can >> take a QOM path) >> - confusing name (but not the first time we've had that issue) >> >> Adding 'qom-path': >> - Pros >> - introspectible >> - JSON expresses everything (we don't have to parse the first >> character of the string to know which style was meant, as the choice of >> key already decided it) >> - Cons >> - Have to implement mutual exclusion ourselves (can't take 'id' and >> 'qom-path' at the same time, and at least one must be specified), unless >> we invent a new way for qapi to express mutual exclusion (there are >> other existing commands that would benefit from such an extension) > > Don't forget having a really long command to type up just to find out > what qom path you need. > > Also the qom path itself is very long. A simple ID is much easier to type out. That's besides the point: IDs can already be specified without this patch. This patch is shoehorning QOM paths into an existing ID argument. Regards, Andreas
On 01/09/2015 11:50, Daniel P. Berrange wrote: > Similarly objects are required to implement the > user-creatable interface > > (qemu) object_del /machine/unattached/device[4] > /machine/unattached/device[4] is not a user-creatable object The question is not whether it's user-creatable, but whether it's user-created. It would probably be bad to delete the iothread that x-data-plane=on creates. IIRC the id is mandatory for all of object_add (HMP), object-add (QMP) and -object, so I would do without the change to object_del. Paolo
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/qapi-schema.json b/qapi-schema.json index 4342a08..bf9ef3a 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1950,7 +1950,7 @@ # # Remove a device from a guest # -# @id: the name of the device +# @id: the name or QOM path of the device # # Returns: Nothing on success # If @id is not a valid device, DeviceNotFound @@ -2121,7 +2121,7 @@ # # Remove a QOM object. # -# @id: the name of the QOM object to remove +# @id: the name or path of the QOM object to remove # # Returns: Nothing on success # Error if @id is not a valid id for a QOM object diff --git a/qdev-monitor.c b/qdev-monitor.c index f9e2d62..295441b 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -785,19 +785,28 @@ 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); return; } + if (!object_dynamic_cast(obj, TYPE_DEVICE)) { + error_setg(errp, "%s is not a hotpluggable device", id); + return; + } + qdev_unplug(DEVICE(obj), errp); } diff --git a/qmp-commands.hx b/qmp-commands.hx index ba630b1..09fc206 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -321,13 +321,18 @@ Remove a device. Arguments: -- "id": the device's ID (json-string) +- "id": the device's ID or QOM path (json-string) Example: -> { "execute": "device_del", "arguments": { "id": "net1" } } <- { "return": {} } +Example: + +-> { "execute": "device_del", "arguments": { "id": "/machine/peripheral-anon/device[0]" } } +<- { "return": {} } + EQMP { @@ -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..b44612a 100644 --- a/qmp.c +++ b/qmp.c @@ -678,16 +678,25 @@ 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; } + if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { + error_setg(errp, "%s is not a user-creatable object", id); + return; + } + if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) { error_setg(errp, "%s is in use, can not be deleted", id); 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. Devices are required to be marked as hotpluggable otherwise an error is raised (qemu) device_del /machine/unattached/device[4] Device 'PIIX3' does not support hotplugging Similarly objects are required to implement the user-creatable interface (qemu) object_del /machine/unattached/device[4] /machine/unattached/device[4] is not a user-creatable object Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- Changed in v3: - Add type checks to avoid assertion failures if user supplied path is not of type device or user-creatable hmp-commands.hx | 6 ++++-- qapi-schema.json | 4 ++-- qdev-monitor.c | 19 ++++++++++++++----- qmp-commands.hx | 13 +++++++++++-- qmp.c | 15 ++++++++++++--- 5 files changed, 43 insertions(+), 14 deletions(-)