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.

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(-)

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
@@ -93,16 +93,30 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
     BlockBackend *blk;
     bool blk_created = false;
     int ret;
+    BlockDriverState *bs;
+    AioContext *ctx;
 
     if (!visit_type_str(v, name, &str, errp)) {
         return;
     }
 
-    /*
-     * TODO Should this really be an error?  If no, the old value
-     * needs to be released before we store the new one.
-     */
-    if (!check_prop_still_unset(obj, name, *ptr, str, errp)) {
+    if (*ptr) {
+        /* BlockBackend alread exists. So, we want to change attached node */
+        blk = *ptr;
+        ctx = blk_get_aio_context(blk);
+        bs = bdrv_lookup_bs(NULL, str, errp);
+        if (!bs) {
+            return;
+        }
+
+        if (ctx != bdrv_get_aio_context(bs)) {
+            error_setg(errp, "Different aio context is not supported for new "
+                       "node");
+        }
+
+        aio_context_acquire(ctx);
+        blk_replace_bs(blk, bs, errp);
+        aio_context_release(ctx);
         return;
     }
 
@@ -114,7 +128,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
 
     blk = blk_by_name(str);
     if (!blk) {
-        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
+        bs = bdrv_lookup_bs(NULL, str, NULL);
         if (bs) {
             /*
              * If the device supports iothreads, it will make sure to move the
@@ -123,8 +137,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
              * aware of iothreads require their BlockBackends to be in the main
              * AioContext.
              */
-            AioContext *ctx = iothread ? bdrv_get_aio_context(bs) :
-                                         qemu_get_aio_context();
+            ctx = iothread ? bdrv_get_aio_context(bs) : qemu_get_aio_context();
             blk = blk_new(ctx, 0, BLK_PERM_ALL);
             blk_created = true;
 
@@ -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,
-- 
2.29.2

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.

> 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?

Max

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]

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


-- 
Best regards,
Vladimir

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

18.05.2021 12:09, Max Reitz wrote:
> 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.”
> 
> ?

Yes, sounds good, thanks

> 
>>
>> 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
> 


-- 
Best regards,
Vladimir