[v2] monitor: allow object_del & device_del to accept QOM paths

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 ++++--
 qapi-schema.json |  4 ++--
 qdev-monitor.c   | 14 +++++++++-----
 qmp-commands.hx  | 13 +++++++++++--
 qmp.c            | 10 +++++++---
 5 files changed, 33 insertions(+), 14 deletions(-)

Changed in v2:

 - Fix stupid typo in qmp-commands.hx
 - Expand parameter docs in qapi-schema.json

On 08/27/2015 10:13 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 ++++--
>  qapi-schema.json |  4 ++--
>  qdev-monitor.c   | 14 +++++++++-----
>  qmp-commands.hx  | 13 +++++++++++--
>  qmp.c            | 10 +++++++---
>  5 files changed, 33 insertions(+), 14 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

On Aug 27, 2015, at 12:13 PM, 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 ++++--
> qapi-schema.json |  4 ++--
> qdev-monitor.c   | 14 +++++++++-----
> qmp-commands.hx  | 13 +++++++++++--
> qmp.c            | 10 +++++++---
> 5 files changed, 33 insertions(+), 14 deletions(-)
> 
> Changed in v2:
> 
> - Fix stupid typo in qmp-commands.hx
> - Expand parameter docs in qapi-schema.json
> 
> 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..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..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..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;
> -- 
> 2.4.3
> 

Your patch do the job, but could be tweaked. Could we make an alias for
"qom-list /machine/peripheral-anon" that is actually easy to remember. Perhaps "info devices". 

Also typing "device_del /machine/peripheral-anon/device[0]" seems unnecessary. It probably 
could be shortened to just "device_del device[0]". 

Thank you.

On 08/27/2015 11:26 AM, Programmingkid wrote:
> 
> On Aug 27, 2015, at 12:13 PM, 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]

> Your patch do the job, but could be tweaked. Could we make an alias for
> "qom-list /machine/peripheral-anon" that is actually easy to remember. Perhaps "info devices". 
> 
> Also typing "device_del /machine/peripheral-anon/device[0]" seems unnecessary. It probably 
> could be shortened to just "device_del device[0]". 

You're welcome to submit follow up patches to make HMP more useful to
you ('info' is already an HMP command, so adding an 'info devices'
submode is a reasonable addition).  However, the important change here
is the QMP change that management apps, like libvirt, will use; and that
interface does not need further syntactic sugar.  The patch specifically
distinguishes between full QOM paths (leading '/') and short names (no
leading '/'); and your proposal to try the QOM path under
/machine/peripheral-anon/ when given just the string 'device[0]' that
did not resolve to a short name is counter to that distinguishing
factor.  Besides, management apps don't care how long a string is.  The
QMP interface should not be changed, and any such simplifications you
write a patch for should be limited to the HMP side.

On 2015/8/28 0:13, 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 ++++--
>  qapi-schema.json |  4 ++--
>  qdev-monitor.c   | 14 +++++++++-----
>  qmp-commands.hx  | 13 +++++++++++--
>  qmp.c            | 10 +++++++---
>  5 files changed, 33 insertions(+), 14 deletions(-)
> 
> Changed in v2:
> 
>  - Fix stupid typo in qmp-commands.hx
>  - Expand parameter docs in qapi-schema.json
> 

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/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..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..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..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;
>

On Thu, Aug 27, 2015 at 01:26:35PM -0400, Programmingkid wrote:
> 
> On Aug 27, 2015, at 12:13 PM, 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 ++++--
> > qapi-schema.json |  4 ++--
> > qdev-monitor.c   | 14 +++++++++-----
> > qmp-commands.hx  | 13 +++++++++++--
> > qmp.c            | 10 +++++++---
> > 5 files changed, 33 insertions(+), 14 deletions(-)
> > 
> > Changed in v2:
> > 
> > - Fix stupid typo in qmp-commands.hx
> > - Expand parameter docs in qapi-schema.json
> > 
> > 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..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..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..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;
> > -- 
> > 2.4.3
> > 
> 
> Your patch do the job, but could be tweaked. Could we make an alias for
> "qom-list /machine/peripheral-anon" that is actually easy to remember. Perhaps "info devices". 

NB, that path is just an example - other paths may be used. It could be
useful to extend one or more 'info' commands to report the ID and/or QOM
path against devices to make the info available in a more friendly manner
than the qom-list command presents.

> Also typing "device_del /machine/peripheral-anon/device[0]" seems unnecessary. It probably 
> could be shortened to just "device_del device[0]".

Note that the /machine/peripheral-anon/ prefix will only be appropriate
for devices where the user did not supply a name. If the user supplied
a name, then the device may be at a different location in the path
hierarchy. Since this is a general purpose command, we must ensure it
covers all usage scenarios, not just the case where the user did not supply
an ID, hence it needs the full path.  Also, using the full path, is key
to ensuring we have a separate namespace for IDs vs paths. If we allowed
an unqualified path, then we'd have to try interpreting it as a path
and then fallback to interpreting it as an ID. This would lead to
non-deterministic behaviour if there was an ID that matched an unqualified
path.

Regards,
Daniel

Gah, missed your v2!  Sorry for the noise on v1.

Still 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>

Thanks for tackling this, it's long overdue!

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

> ---
>  hmp-commands.hx  |  6 ++++--
>  qapi-schema.json |  4 ++--
>  qdev-monitor.c   | 14 +++++++++-----
>  qmp-commands.hx  | 13 +++++++++++--
>  qmp.c            | 10 +++++++---
>  5 files changed, 33 insertions(+), 14 deletions(-)
>
> Changed in v2:
>
>  - Fix stupid typo in qmp-commands.hx
>  - Expand parameter docs in qapi-schema.json
>
> 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..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);
> +    }

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);
> 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..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;

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.  Could also be touched up on commit.

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

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