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" <address@hidden> 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 <address@hidden>
>> ---
>> 
>> 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 <address@hidden>
> 
> 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?