Re: [PATCH 04/21] qdev: allow setting drive property for realized device

[Max Reitz <mreitz@redhat.com>]

On 17.05.21 20:09, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2021 18:48, Max Reitz wrote:
>> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
>>> We need an ability to insert filters above top block node, attached to
>>> block device. It can't be achieved with blockdev-reopen command. So, we
>>> want do it with help of qom-set.
>>>
>>> Intended usage:
>>>
>>> 1. blockdev-add, creating the filter, which child is at top node A,
>>>     attached to some guest block device.
>>
>> Is a “not” missing here, i.e. “not attached to any guest block 
>> device”?  I would have thought one would create a filtered tree that 
>> is not in use by any frontend, so that the filter need not take any 
>> permissions.
> 
> node A is attached.
> 
> So, we have [blk] --root->  [A}
> 
> And want to insert a filter between blk and A.
> 
> We do
> 
> 1.
> 
> [filter] --file--\
>                   v
> [blk] --root-->  [A]

Oh, so you mean node A is attached to a guest device.  The sentence 
sounded to me like the newly created filter tree were attached to it.

Yes, that’s how I expected it to be.  I just find the sentence not quite 
clear, because I found it ambiguous which node the “attached to some 
guest block device” refers to.

Perhaps:
“Intended usage:

Assume there is a node A that is attached to some guest device.
1. blockdev-add to create a filter node B that has A as its child.
2. qom-set to change the node attached to the guest device’s 
BlockBackend from A to B.”

?

> 
> 2.
> 
> [blk] --root--> [filer] --file--> [A]
> 
>>
>>> 2. qom-set, to change bs attached to root blk from original node to
>>>     newly create filter.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   hw/core/qdev-properties-system.c | 30 ++++++++++++++++++++++--------
>>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> Looks good, just one question: (well, two, one was above)
>>
>>> diff --git a/hw/core/qdev-properties-system.c 
>>> b/hw/core/qdev-properties-system.c
>>> index 2760c21f11..7d97562654 100644
>>> --- a/hw/core/qdev-properties-system.c
>>> +++ b/hw/core/qdev-properties-system.c
>>
>> [...]
>>
>>> @@ -196,6 +209,7 @@ static void release_drive(Object *obj, const char 
>>> *name, void *opaque)
>>>   const PropertyInfo qdev_prop_drive = {
>>>       .name  = "str",
>>>       .description = "Node name or ID of a block device to use as a 
>>> backend",
>>> +    .realized_set_allowed = true,
>>>       .get   = get_drive,
>>>       .set   = set_drive,
>>>       .release = release_drive,
>>
>> Why not for qdev_prop_drive_iothread?
>>
> 
> Hmm, the only reason is that I missed that part of architecture around 
> here, I'm new to qdev code. Will add with next version

OK.  (I just saw it because it was right below this structure, too, it 
isn’t like I actually know what I’m saying.)

Max