[Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Daniel P. Berrange]

Currently device_del requires that the client provide the
device short ID. device_add allows devices may be created
without giving an ID, at which point there is no way to
delete them with device_del. The QOM object path, however,
provides an alternative way to identify the devices.

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]

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Changes in v4:

 - Drop additions to object_del, since that can be misused
   to delete internally created devices, and object_create
   mandates an ID parameter.

 hmp-commands.hx  |  3 ++-
 qapi-schema.json |  2 +-
 qdev-monitor.c   | 19 ++++++++++++++-----
 qmp-commands.hx  |  7 ++++++-
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 286dcc7..e56373b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -676,7 +676,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
 
     {
diff --git a/qapi-schema.json b/qapi-schema.json
index 67fef37..273a906 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
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 9848fd8..d3b4c7f 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
 
     {
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On 09/11/2015 06:33 AM, Daniel P. Berrange wrote:
> Currently device_del requires that the client provide the
> device short ID. device_add allows devices may be created
> without giving an ID, at which point there is no way to
> delete them with device_del. The QOM object path, however,
> provides an alternative way to identify the devices.
> 
> 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]
> 
> 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
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

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: 604 bytes --]

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> Currently device_del requires that the client provide the
> device short ID. device_add allows devices may be created

"allows devices to be created"

Could perhaps be touched up on commit.

> without giving an ID, at which point there is no way to
> delete them with device_del. The QOM object path, however,
> provides an alternative way to identify the devices.
>
> 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]
>
> 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
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>
> Changes in v4:
>
>  - Drop additions to object_del, since that can be misused
>    to delete internally created devices, and object_create
>    mandates an ID parameter.
>
>  hmp-commands.hx  |  3 ++-
>  qapi-schema.json |  2 +-
>  qdev-monitor.c   | 19 ++++++++++++++-----
>  qmp-commands.hx  |  7 ++++++-
>  4 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 286dcc7..e56373b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -676,7 +676,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
>  
>      {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 67fef37..273a906 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
> 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);
> +    }

Blank line deleted right here, could perhaps be fixed up on commit.

>      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;
> +    }
> +

This error can happen only with a path, because with an ID, we use

        obj = object_resolve_path_type(path, TYPE_DEVICE, NULL);

We could do similarly for paths, but doing it this way results in a
better error message.

>      qdev_unplug(DEVICE(obj), errp);
>  }
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9848fd8..d3b4c7f 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
>  
>      {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

If neither Andreas nor Paolo objects, I'd be willing to take this
through my tree.

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Programmingkid]

On Sep 14, 2015, at 11:53 AM, Markus Armbruster wrote:

> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
>> Currently device_del requires that the client provide the
>> device short ID. device_add allows devices may be created
> 
> "allows devices to be created"
> 
> Could perhaps be touched up on commit.
> 
>> without giving an ID, at which point there is no way to
>> delete them with device_del. The QOM object path, however,
>> provides an alternative way to identify the devices.
>> 
>> 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]
>> 
>> 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
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>> 
>> Changes in v4:
>> 
>> - Drop additions to object_del, since that can be misused
>>   to delete internally created devices, and object_create
>>   mandates an ID parameter.
>> 
>> hmp-commands.hx  |  3 ++-
>> qapi-schema.json |  2 +-
>> qdev-monitor.c   | 19 ++++++++++++++-----
>> qmp-commands.hx  |  7 ++++++-
>> 4 files changed, 23 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 286dcc7..e56373b 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -676,7 +676,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
>> 
>>     {
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 67fef37..273a906 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
>> 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);
>> +    }
> 
> Blank line deleted right here, could perhaps be fixed up on commit.
> 
>>     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;
>> +    }
>> +
> 
> This error can happen only with a path, because with an ID, we use
> 
>        obj = object_resolve_path_type(path, TYPE_DEVICE, NULL);
> 
> We could do similarly for paths, but doing it this way results in a
> better error message.
> 
>>     qdev_unplug(DEVICE(obj), errp);
>> }
>> 
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 9848fd8..d3b4c7f 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
>> 
>>     {
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> If neither Andreas nor Paolo objects, I'd be willing to take this
> through my tree.

If you do accept this patch, would you still be willing to
accept an auto-generated ID patch still?

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

On 09/14/2015 10:17 AM, Programmingkid wrote:
> 
> On Sep 14, 2015, at 11:53 AM, Markus Armbruster wrote:
> 
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>
>>> Currently device_del requires that the client provide the
>>> device short ID. device_add allows devices may be created
>>
>> "allows devices to be created"
>>
>> Could perhaps be touched up on commit.
>>
>>> without giving an ID, at which point there is no way to
>>> delete them with device_del. The QOM object path, however,
>>> provides an alternative way to identify the devices.
>>>

>> If neither Andreas nor Paolo objects, I'd be willing to take this
>> through my tree.
> 
> If you do accept this patch, would you still be willing to
> accept an auto-generated ID patch still?

The two are orthogonal.  Deleting by QOM path is useful even if all
devices are given an id, and auto-generating an id may be useful to
human users (even though libvirt won't rely on it) regardless of whether
deleting devices by QOM path is in the tree.

But you are worrying about nothing; although Kevin is waiting for a v3,
he has already mentioned on v2 that he is ready to queue auto-generation
once it works:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00877.html

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> On Sep 14, 2015, at 11:53 AM, Markus Armbruster wrote:
[...]
>> If neither Andreas nor Paolo objects, I'd be willing to take this
>> through my tree.
>
> If you do accept this patch, would you still be willing to
> accept an auto-generated ID patch still?

I'm on record saying

    I believe [this patch] makes sense no matter what we do about device
    IDs (see thread "Should we auto-generate IDs?").

This patch weakens *one* reason for auto-generated IDs: device_del
without ID is impossible before, and merely a bit inconvenient after.  I
haven't been able to keep up with the discussion on auto-generated IDs,
so I can't currently judge how strong the other reasons are.

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Programmingkid]

On Sep 14, 2015, at 12:41 PM, Eric Blake wrote:

> On 09/14/2015 10:17 AM, Programmingkid wrote:
>> 
>> On Sep 14, 2015, at 11:53 AM, Markus Armbruster wrote:
>> 
>>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>> 
>>>> Currently device_del requires that the client provide the
>>>> device short ID. device_add allows devices may be created
>>> 
>>> "allows devices to be created"
>>> 
>>> Could perhaps be touched up on commit.
>>> 
>>>> without giving an ID, at which point there is no way to
>>>> delete them with device_del. The QOM object path, however,
>>>> provides an alternative way to identify the devices.
>>>> 
> 
>>> If neither Andreas nor Paolo objects, I'd be willing to take this
>>> through my tree.
>> 
>> If you do accept this patch, would you still be willing to
>> accept an auto-generated ID patch still?
> 
> The two are orthogonal.  Deleting by QOM path is useful even if all
> devices are given an id, and auto-generating an id may be useful to
> human users (even though libvirt won't rely on it) regardless of whether
> deleting devices by QOM path is in the tree.
> 
> But you are worrying about nothing; although Kevin is waiting for a v3,
> he has already mentioned on v2 that he is ready to queue auto-generation
> once it works:
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00877.html

It has been a while since Kevin asked for a version 3. I wonder if the author
of the patch knows about the version 3 request. 

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Programmingkid]

On Sep 14, 2015, at 12:43 PM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> On Sep 14, 2015, at 11:53 AM, Markus Armbruster wrote:
> [...]
>>> If neither Andreas nor Paolo objects, I'd be willing to take this
>>> through my tree.
>> 
>> If you do accept this patch, would you still be willing to
>> accept an auto-generated ID patch still?
> 
> I'm on record saying
> 
>    I believe [this patch] makes sense no matter what we do about device
>    IDs (see thread "Should we auto-generate IDs?").
> 
> This patch weakens *one* reason for auto-generated IDs: device_del
> without ID is impossible before, and merely a bit inconvenient after.  I
> haven't been able to keep up with the discussion on auto-generated IDs,
> so I can't currently judge how strong the other reasons are.

I think the main reason to use auto-generated IDs is because they can be
much easier to type than a full QOM path. I know the device_del patch
weaken the reason for auto-generated IDs, it doesn't have to eliminate it. 
We are allowed to have both QOM paths and auto-generated IDs. 

Re: [Qemu-devel] [PATCH v4] monitor: allow device_del to accept QOM paths

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On 09/14/2015 10:49 AM, Programmingkid wrote:
>> But you are worrying about nothing; although Kevin is waiting for a v3,
>> he has already mentioned on v2 that he is ready to queue auto-generation
>> once it works:
>> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00877.html
> 
> It has been a while since Kevin asked for a version 3. I wonder if the author
> of the patch knows about the version 3 request. 

You're worrying about nothing (not to mention worrying on the wrong
thread).  The Red Hat engineers (including Jeff [as author of the thread
you are worrying about], Kevin [as the maintainer that plans to take
that patch], Paolo, Markus, myself [each listed on cc on this thread],
and several others on the list) are fairly good about communicating with
one another whenever a piece of work is blocking on someone else's
actions, even if some of that communication occurs on private IRC
channels instead of the list (to cut down on list traffic that is not as
interesting to others).  And from personal experience, frequent
contributors on this list can be trusted to revise their patches as
requested, without having to be prodded.  And Red Hat has no monopoly on
frequent contributors; Andreas [who is also on cc for this thread] is
yet another example, out of many, of someone that will follow through
with addressing review comments.

Remember, just because patches don't land in 24 hours doesn't mean they
aren't happening.  Also, engineers may have multiple things on their
plate, so while they will eventually get to responding to review
comments, they may have other higher-priority tasks to attend to first.
 The 2.5 release freeze is still several weeks off, and it doesn't
matter whether a patch lands today or in two weeks, if it is aiming to
make it into the 2.5 release.  Please exercise some patience.  And while
pings are sometimes appropriate, it is also reasonable to wait a least a
week from the last activity on a thread, and then to ping on the thread
you are worried about, rather than to side-track an unrelated thread
where you may not even be reaching the right people.

-- 
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: 604 bytes --]