[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] monitor: allow object_del & device_del to accep
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] monitor: allow object_del & device_del to accept QOM paths |
Date: |
Fri, 28 Aug 2015 14:53:41 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Copying Andreas and Paolo for QOM expertise.
"Daniel P. Berrange" <address@hidden> 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 <address@hidden>
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!