Re: [Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Markus Armbruster]

Jan Kiszka <address@hidden> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <address@hidden> writes:
>> 
>>> Luiz Capitulino wrote:
>>>> On Sun, 23 May 2010 12:59:19 +0200
>>>> Jan Kiszka <address@hidden> wrote:
>>>>
>>>>> From: Jan Kiszka <address@hidden>
>>>>>
>>>>> 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.