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

[Max Reitz]

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