diff mbox

[v3,06/17] qdev: Allow device specification by qtree path for device_del

Message ID cb2d6240768804903ddb56a095c6706267070466.1274612367.git.jan.kiszka@web.de
State New
Headers show

Commit Message

Jan Kiszka May 23, 2010, 10:59 a.m. UTC
From: Jan Kiszka <jan.kiszka@siemens.com>

Allow to specify the device to be removed via device_del not only by ID
but also by its full or abbreviated qtree path. For this purpose,
qdev_find is introduced which combines walking the qtree with searching
for device IDs if required.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c       |   49 +++++++++++++++++++++++++++++++++++++++++++------
 qemu-monitor.hx |   10 +++++-----
 2 files changed, 48 insertions(+), 11 deletions(-)

Comments

Luiz Capitulino May 27, 2010, 7:36 p.m. UTC | #1
On Sun, 23 May 2010 12:59:19 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Allow to specify the device to be removed via device_del not only by ID
> but also by its full or abbreviated qtree path. For this purpose,
> qdev_find is introduced which combines walking the qtree with searching
> for device IDs if required.

 [...]

>  Arguments:
>  
> -- "id": the device's ID (json-string)
> +- "path": the device's qtree path or unique ID (json-string)
>  
>  Example:
>  
> --> { "execute": "device_del", "arguments": { "id": "net1" } }
> +-> { "execute": "device_del", "arguments": { "path": "net1" } }

 Doesn't seem like a good change to me, besides being incompatible[1] we
shouldn't overload arguments this way in QMP as overloading leads to
interface degradation (harder to use, understand, maintain).

 Maybe we could have both arguments as optional, but one must be passed.

[1] It's 'legal' to break the protocol before 0.13, but this has to be
    coordinated with libvirt so we should have a good reason to do this

>  <- { "return": {} }
>  
>  EQMP
Jan Kiszka May 27, 2010, 10:19 p.m. UTC | #2
Luiz Capitulino wrote:
> On Sun, 23 May 2010 12:59:19 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Allow to specify the device to be removed via device_del not only by ID
>> but also by its full or abbreviated qtree path. For this purpose,
>> qdev_find is introduced which combines walking the qtree with searching
>> for device IDs if required.
> 
>  [...]
> 
>>  Arguments:
>>  
>> -- "id": the device's ID (json-string)
>> +- "path": the device's qtree path or unique ID (json-string)
>>  
>>  Example:
>>  
>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
> 
>  Doesn't seem like a good change to me, besides being incompatible[1] we
> shouldn't overload arguments this way in QMP as overloading leads to
> interface degradation (harder to use, understand, maintain).

It's not overloaded, think of an ID as a (weak) symbolic link in the
qtree filesystem. The advantage of basing everything on top of full or
abbreviated qtree paths is that IDs are not always assigned, paths are.

> 
>  Maybe we could have both arguments as optional, but one must be passed.

This would at least require some way to keep the proposed unified path
specification for the human monitor (having separate arguments there is
really unhandy).

> 
> [1] It's 'legal' to break the protocol before 0.13, but this has to be
>     coordinated with libvirt so we should have a good reason to do this
> 
>>  <- { "return": {} }
>>  
>>  EQMP
> 

Jan
Luiz Capitulino May 28, 2010, 1:43 p.m. UTC | #3
On Fri, 28 May 2010 00:19:48 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Sun, 23 May 2010 12:59:19 +0200
> > Jan Kiszka <jan.kiszka@web.de> wrote:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Allow to specify the device to be removed via device_del not only by ID
> >> but also by its full or abbreviated qtree path. For this purpose,
> >> qdev_find is introduced which combines walking the qtree with searching
> >> for device IDs if required.
> > 
> >  [...]
> > 
> >>  Arguments:
> >>  
> >> -- "id": the device's ID (json-string)
> >> +- "path": the device's qtree path or unique ID (json-string)
> >>  
> >>  Example:
> >>  
> >> --> { "execute": "device_del", "arguments": { "id": "net1" } }
> >> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
> > 
> >  Doesn't seem like a good change to me, besides being incompatible[1] we
> > shouldn't overload arguments this way in QMP as overloading leads to
> > interface degradation (harder to use, understand, maintain).
> 
> It's not overloaded, think of an ID as a (weak) symbolic link in the
> qtree filesystem. The advantage of basing everything on top of full or
> abbreviated qtree paths is that IDs are not always assigned, paths are.

 You mean there're cases where an ID is not assigned? I didn't know that,
I thought they were always assigned, at least via QMP.

 In any case, my main question here is that if this change really makes
sense for QMP or if it's something for HMP where we can have features
like tab completion.

> >  Maybe we could have both arguments as optional, but one must be passed.
> 
> This would at least require some way to keep the proposed unified path
> specification for the human monitor (having separate arguments there is
> really unhandy).

 Agreed, perhaps we have to decouple QMP and HMP in the way proposed by
Anthony: the HMP should work by making QMP calls (IIUC).

 This way HMP can accept whatever arguments make sense for it, but then
it should transform them in a QMP call.

 This is a long term project though.
Jan Kiszka May 28, 2010, 2:16 p.m. UTC | #4
Luiz Capitulino wrote:
> On Fri, 28 May 2010 00:19:48 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> Luiz Capitulino wrote:
>>> On Sun, 23 May 2010 12:59:19 +0200
>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Allow to specify the device to be removed via device_del not only by ID
>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>> qdev_find is introduced which combines walking the qtree with searching
>>>> for device IDs if required.
>>>  [...]
>>>
>>>>  Arguments:
>>>>  
>>>> -- "id": the device's ID (json-string)
>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>  
>>>>  Example:
>>>>  
>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>> shouldn't overload arguments this way in QMP as overloading leads to
>>> interface degradation (harder to use, understand, maintain).
>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>> qtree filesystem. The advantage of basing everything on top of full or
>> abbreviated qtree paths is that IDs are not always assigned, paths are.
> 
>  You mean there're cases where an ID is not assigned? I didn't know that,
> I thought they were always assigned, at least via QMP.

Yes, platform devices or anything else that is auto-instantiated does
not have an ID. Granted, you normally don't want or even can remove such
devices, but you can perfectly display them. And someone asked for
unifying the paths accepted by device_show and device_del.

> 
>  In any case, my main question here is that if this change really makes
> sense for QMP or if it's something for HMP where we can have features
> like tab completion.

I don't think QMP would suffer from having separate arguments for ID and
qtree paths. We just need a way to map them on the same for HMP, ie. we
need argument types that only show up in one of both domains.

> 
>>>  Maybe we could have both arguments as optional, but one must be passed.
>> This would at least require some way to keep the proposed unified path
>> specification for the human monitor (having separate arguments there is
>> really unhandy).
> 
>  Agreed, perhaps we have to decouple QMP and HMP in the way proposed by
> Anthony: the HMP should work by making QMP calls (IIUC).
> 
>  This way HMP can accept whatever arguments make sense for it, but then
> it should transform them in a QMP call.
> 
>  This is a long term project though.

But the changes I propose already take effect today. We need at least an
interim solution.

We could postpone the device_del HMP format change e.g., but that would
take away some usability and leave criticized inconsistency behind.

Hmm, or we could simply introduce optional .hmp_args_type and
.hmp_params. HMP would favor them when provided by a command, otherwise
fall back to the standard params. Doesn't sound awfully complex and
invasive.

Jan
Markus Armbruster May 28, 2010, 2:40 p.m. UTC | #5
Jan Kiszka <jan.kiszka@web.de> writes:

> Luiz Capitulino wrote:
>> On Sun, 23 May 2010 12:59:19 +0200
>> Jan Kiszka <jan.kiszka@web.de> wrote:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Allow to specify the device to be removed via device_del not only by ID
>>> but also by its full or abbreviated qtree path. For this purpose,
>>> qdev_find is introduced which combines walking the qtree with searching
>>> for device IDs if required.
>> 
>>  [...]
>> 
>>>  Arguments:
>>>  
>>> -- "id": the device's ID (json-string)
>>> +- "path": the device's qtree path or unique ID (json-string)
>>>  
>>>  Example:
>>>  
>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>> 
>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>> shouldn't overload arguments this way in QMP as overloading leads to
>> interface degradation (harder to use, understand, maintain).
>
> It's not overloaded, think of an ID as a (weak) symbolic link in the
> qtree filesystem. The advantage of basing everything on top of full or
> abbreviated qtree paths is that IDs are not always assigned, paths are.

As long as your patch doesn't change the interpretation of IDs, we can
keep the old name.

The recent review of QMP documentation may lead to a "clean up bad
names" flag day.  One more wouldn't make it worse, I guess.

>>  Maybe we could have both arguments as optional, but one must be passed.
>
> This would at least require some way to keep the proposed unified path
> specification for the human monitor (having separate arguments there is
> really unhandy).

Correct.

It would be nice to have device_del support paths in addition to IDs.
I'd expect management tools to slap IDs on everything, so they won't
care, but human users do.

As far as I know, we have two places where we let the user name a node
in the qtree: device_add bus=X and device_del X.  The former names a
bus, the latter a device.  But both are nodes in the same tree, so
consistency is in order.

Only devices have user-specified IDs.  Buses have names assigned by the
system.  Unique names, hopefully.

If the user doesn't specify a device ID, the driver name is used
instead.  If you put multiple instances of the same device on the same
bus, they have the *same* path.  For instance, here's a snippet of info
qtree after adding two usb-mouse:

      dev: piix3-usb-uhci, id ""
        bus-prop: addr = 01.2
        bus-prop: romfile = <null>
        bus-prop: rombar = 1
        class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
        bar 4: i/o at 0xffffffffffffffff [0x1e]
        bus: usb.0
          type USB
          dev: usb-hub, id ""
            addr 0.0, speed 12, name QEMU USB Hub, attached
          dev: usb-mouse, id "no2"
            addr 0.0, speed 12, name QEMU USB Mouse, attached
          dev: usb-mouse, id ""
            addr 0.0, speed 12, name QEMU USB Mouse, attached

Both devices have the same full path
/i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
Which one does your code pick?  Shouldn't it refuse to pick?

By the way, you *can* put '/' in IDs.  I call that a bug.

>> [1] It's 'legal' to break the protocol before 0.13, but this has to be
>>     coordinated with libvirt so we should have a good reason to do this
>> 
>>>  <- { "return": {} }
>>>  
>>>  EQMP
>> 
>
> Jan
Jan Kiszka May 28, 2010, 2:56 p.m. UTC | #6
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> Luiz Capitulino wrote:
>>> On Sun, 23 May 2010 12:59:19 +0200
>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Allow to specify the device to be removed via device_del not only by ID
>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>> qdev_find is introduced which combines walking the qtree with searching
>>>> for device IDs if required.
>>>  [...]
>>>
>>>>  Arguments:
>>>>  
>>>> -- "id": the device's ID (json-string)
>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>  
>>>>  Example:
>>>>  
>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>> shouldn't overload arguments this way in QMP as overloading leads to
>>> interface degradation (harder to use, understand, maintain).
>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>> qtree filesystem. The advantage of basing everything on top of full or
>> abbreviated qtree paths is that IDs are not always assigned, paths are.
> 
> As long as your patch doesn't change the interpretation of IDs, we can
> keep the old name.
> 
> The recent review of QMP documentation may lead to a "clean up bad
> names" flag day.  One more wouldn't make it worse, I guess.
> 
>>>  Maybe we could have both arguments as optional, but one must be passed.
>> This would at least require some way to keep the proposed unified path
>> specification for the human monitor (having separate arguments there is
>> really unhandy).
> 
> Correct.
> 
> It would be nice to have device_del support paths in addition to IDs.
> I'd expect management tools to slap IDs on everything, so they won't
> care, but human users do.
> 
> As far as I know, we have two places where we let the user name a node
> in the qtree: device_add bus=X and device_del X.  The former names a
> bus, the latter a device.  But both are nodes in the same tree, so
> consistency is in order.
> 
> Only devices have user-specified IDs.  Buses have names assigned by the
> system.  Unique names, hopefully.

...but not necessarily. The bus name device_add accepts can also be a
full, thus unambiguous path.

> 
> If the user doesn't specify a device ID, the driver name is used
> instead.  If you put multiple instances of the same device on the same
> bus, they have the *same* path.  For instance, here's a snippet of info
> qtree after adding two usb-mouse:
> 
>       dev: piix3-usb-uhci, id ""
>         bus-prop: addr = 01.2
>         bus-prop: romfile = <null>
>         bus-prop: rombar = 1
>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>         bus: usb.0
>           type USB
>           dev: usb-hub, id ""
>             addr 0.0, speed 12, name QEMU USB Hub, attached
>           dev: usb-mouse, id "no2"
>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>           dev: usb-mouse, id ""
>             addr 0.0, speed 12, name QEMU USB Mouse, attached
> 
> Both devices have the same full path
> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
> Which one does your code pick?  Shouldn't it refuse to pick?

Patch 3 of this series resolves this as follows:

usb-mouse[.0] -> first listed instance
usb-mouse.1   -> second instance
...

We should probably include this numbering in the qtree dump, I guess.

> 
> By the way, you *can* put '/' in IDs.  I call that a bug.

Even if we prevent this, IDs can still collide with abbreviated device
or bus paths. Therefore I give paths precedence over IDs in patch 4.

Jan
Markus Armbruster May 29, 2010, 8:05 a.m. UTC | #7
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> Luiz Capitulino wrote:
>>>> On Sun, 23 May 2010 12:59:19 +0200
>>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Allow to specify the device to be removed via device_del not only by ID
>>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>>> qdev_find is introduced which combines walking the qtree with searching
>>>>> for device IDs if required.
>>>>  [...]
>>>>
>>>>>  Arguments:
>>>>>  
>>>>> -- "id": the device's ID (json-string)
>>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>>  
>>>>>  Example:
>>>>>  
>>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>>> shouldn't overload arguments this way in QMP as overloading leads to
>>>> interface degradation (harder to use, understand, maintain).
>>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>>> qtree filesystem. The advantage of basing everything on top of full or
>>> abbreviated qtree paths is that IDs are not always assigned, paths are.
>> 
>> As long as your patch doesn't change the interpretation of IDs, we can
>> keep the old name.
>> 
>> The recent review of QMP documentation may lead to a "clean up bad
>> names" flag day.  One more wouldn't make it worse, I guess.
>> 
>>>>  Maybe we could have both arguments as optional, but one must be passed.
>>> This would at least require some way to keep the proposed unified path
>>> specification for the human monitor (having separate arguments there is
>>> really unhandy).
>> 
>> Correct.
>> 
>> It would be nice to have device_del support paths in addition to IDs.
>> I'd expect management tools to slap IDs on everything, so they won't
>> care, but human users do.
>> 
>> As far as I know, we have two places where we let the user name a node
>> in the qtree: device_add bus=X and device_del X.  The former names a
>> bus, the latter a device.  But both are nodes in the same tree, so
>> consistency is in order.
>> 
>> Only devices have user-specified IDs.  Buses have names assigned by the
>> system.  Unique names, hopefully.
>
> ...but not necessarily. The bus name device_add accepts can also be a
> full, thus unambiguous path.
>
>> 
>> If the user doesn't specify a device ID, the driver name is used
>> instead.  If you put multiple instances of the same device on the same
>> bus, they have the *same* path.  For instance, here's a snippet of info
>> qtree after adding two usb-mouse:
>> 
>>       dev: piix3-usb-uhci, id ""
>>         bus-prop: addr = 01.2
>>         bus-prop: romfile = <null>
>>         bus-prop: rombar = 1
>>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>>         bus: usb.0
>>           type USB
>>           dev: usb-hub, id ""
>>             addr 0.0, speed 12, name QEMU USB Hub, attached
>>           dev: usb-mouse, id "no2"
>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>           dev: usb-mouse, id ""
>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>> 
>> Both devices have the same full path
>> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
>> Which one does your code pick?  Shouldn't it refuse to pick?
>
> Patch 3 of this series resolves this as follows:
>
> usb-mouse[.0] -> first listed instance
> usb-mouse.1   -> second instance
> ...
>
> We should probably include this numbering in the qtree dump, I guess.
>
>> 
>> By the way, you *can* put '/' in IDs.  I call that a bug.
>
> Even if we prevent this, IDs can still collide with abbreviated device
> or bus paths. Therefore I give paths precedence over IDs in patch 4.

You're fixing problems in the overly complex and subtle path name lookup
by making it more complex and subtle.  Let's make it simple and
straightforward instead.
Jan Kiszka May 29, 2010, 8:16 a.m. UTC | #8
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> Luiz Capitulino wrote:
>>>>> On Sun, 23 May 2010 12:59:19 +0200
>>>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> Allow to specify the device to be removed via device_del not only by ID
>>>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>>>> qdev_find is introduced which combines walking the qtree with searching
>>>>>> for device IDs if required.
>>>>>  [...]
>>>>>
>>>>>>  Arguments:
>>>>>>  
>>>>>> -- "id": the device's ID (json-string)
>>>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>>>  
>>>>>>  Example:
>>>>>>  
>>>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>>>> shouldn't overload arguments this way in QMP as overloading leads to
>>>>> interface degradation (harder to use, understand, maintain).
>>>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>>>> qtree filesystem. The advantage of basing everything on top of full or
>>>> abbreviated qtree paths is that IDs are not always assigned, paths are.
>>> As long as your patch doesn't change the interpretation of IDs, we can
>>> keep the old name.
>>>
>>> The recent review of QMP documentation may lead to a "clean up bad
>>> names" flag day.  One more wouldn't make it worse, I guess.
>>>
>>>>>  Maybe we could have both arguments as optional, but one must be passed.
>>>> This would at least require some way to keep the proposed unified path
>>>> specification for the human monitor (having separate arguments there is
>>>> really unhandy).
>>> Correct.
>>>
>>> It would be nice to have device_del support paths in addition to IDs.
>>> I'd expect management tools to slap IDs on everything, so they won't
>>> care, but human users do.
>>>
>>> As far as I know, we have two places where we let the user name a node
>>> in the qtree: device_add bus=X and device_del X.  The former names a
>>> bus, the latter a device.  But both are nodes in the same tree, so
>>> consistency is in order.
>>>
>>> Only devices have user-specified IDs.  Buses have names assigned by the
>>> system.  Unique names, hopefully.
>> ...but not necessarily. The bus name device_add accepts can also be a
>> full, thus unambiguous path.
>>
>>> If the user doesn't specify a device ID, the driver name is used
>>> instead.  If you put multiple instances of the same device on the same
>>> bus, they have the *same* path.  For instance, here's a snippet of info
>>> qtree after adding two usb-mouse:
>>>
>>>       dev: piix3-usb-uhci, id ""
>>>         bus-prop: addr = 01.2
>>>         bus-prop: romfile = <null>
>>>         bus-prop: rombar = 1
>>>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>>>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>>>         bus: usb.0
>>>           type USB
>>>           dev: usb-hub, id ""
>>>             addr 0.0, speed 12, name QEMU USB Hub, attached
>>>           dev: usb-mouse, id "no2"
>>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>>           dev: usb-mouse, id ""
>>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>>
>>> Both devices have the same full path
>>> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
>>> Which one does your code pick?  Shouldn't it refuse to pick?
>> Patch 3 of this series resolves this as follows:
>>
>> usb-mouse[.0] -> first listed instance
>> usb-mouse.1   -> second instance
>> ...
>>
>> We should probably include this numbering in the qtree dump, I guess.
>>
>>> By the way, you *can* put '/' in IDs.  I call that a bug.
>> Even if we prevent this, IDs can still collide with abbreviated device
>> or bus paths. Therefore I give paths precedence over IDs in patch 4.
> 
> You're fixing problems in the overly complex and subtle path name lookup
> by making it more complex and subtle.  Let's make it simple and
> straightforward instead.

I have no problem with ripping out all of those abbreviations, requiring
the user to either specify a '/'-less unique ID or a full qtree path
that must start with a '/'. If paths get long, we now have interactive
completions.

Jan
diff mbox

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index b3d375a..df945ed 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -469,7 +469,7 @@  static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
 {
     DeviceState *dev, *ret;
     BusState *child;
@@ -478,7 +478,7 @@  static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
         if (dev->id && strcmp(dev->id, id) == 0)
             return dev;
         QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_recursive(child, id);
+            ret = qdev_find_id_recursive(child, id);
             if (ret) {
                 return ret;
             }
@@ -666,6 +666,43 @@  search_dev_bus:
     }
 }
 
+static DeviceState *qdev_find(const char *path)
+{
+    const char *dev_name;
+    DeviceState *dev;
+    char *bus_path;
+    BusState *bus;
+
+    dev_name = strrchr(path, '/');
+    if (!dev_name) {
+        bus = main_system_bus;
+        dev_name = path;
+    } else {
+        dev_name++;
+        bus_path = qemu_strdup(path);
+        bus_path[dev_name - path] = 0;
+
+        bus = qbus_find(bus_path);
+        qemu_free(bus_path);
+
+        if (!bus) {
+            /* qbus_find already reported the error */
+            return NULL;
+        }
+    }
+    dev = qbus_find_dev(bus, dev_name);
+    if (!dev) {
+        dev = qdev_find_id_recursive(main_system_bus, path);
+        if (!dev) {
+            qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
+            if (!monitor_cur_is_qmp()) {
+                qbus_list_dev(bus);
+            }
+        }
+    }
+    return dev;
+}
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name)
 {
@@ -824,12 +861,12 @@  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    const char *id = qdict_get_str(qdict, "id");
+    const char *path = qdict_get_str(qdict, "path");
     DeviceState *dev;
 
-    dev = qdev_find_recursive(main_system_bus, id);
-    if (NULL == dev) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+    dev = qdev_find(path);
+    if (!dev) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, path);
         return -1;
     }
     return qdev_unplug(dev);
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index c8f1789..754d71e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -703,7 +703,7 @@  EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "id:s",
+        .args_type  = "path:s",
         .params     = "device",
         .help       = "remove device",
         .user_print = monitor_user_noop,
@@ -711,10 +711,10 @@  EQMP
     },
 
 STEXI
-@item device_del @var{id}
+@item device_del @var{path}
 @findex device_del
 
-Remove device @var{id}.
+Remove device @var{path}.
 ETEXI
 SQMP
 device_del
@@ -724,11 +724,11 @@  Remove a device.
 
 Arguments:
 
-- "id": the device's ID (json-string)
+- "path": the device's qtree path or unique ID (json-string)
 
 Example:
 
--> { "execute": "device_del", "arguments": { "id": "net1" } }
+-> { "execute": "device_del", "arguments": { "path": "net1" } }
 <- { "return": {} }
 
 EQMP