Re: [PATCH 16/21] block/copy-before-write: cbw_init(): use options

[Max Reitz <mreitz@redhat.com>]

On 18.05.21 16:24, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2021 16:56, Max Reitz wrote:
>> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
>>> One more step closer to .bdrv_open(): use options instead of plain
>>> arguments. Move to bdrv_open_child() calls, native for drive open
>>> handlers.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/copy-before-write.c | 37 ++++++++++++++++++++-----------------
>>>   1 file changed, 20 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
>>> index ddd79b3686..9ff1bf676c 100644
>>> --- a/block/copy-before-write.c
>>> +++ b/block/copy-before-write.c
>>> @@ -144,27 +144,20 @@ static void cbw_child_perm(BlockDriverState 
>>> *bs, BdrvChild *c,
>>>       }
>>>   }
>>> -static int cbw_init(BlockDriverState *bs, BlockDriverState *source,
>>> -                    BlockDriverState *target, bool compress, Error 
>>> **errp)
>>> +static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp)
>>>   {
>>>       BDRVCopyBeforeWriteState *s = bs->opaque;
>>> -    bdrv_ref(target);
>>> -    s->target = bdrv_attach_child(bs, target, "target", &child_of_bds,
>>> -                                  BDRV_CHILD_DATA, errp);
>>> -    if (!s->target) {
>>> -        error_prepend(errp, "Cannot attach target child: ");
>>> -        bdrv_unref(target);
>>> +    bs->file = bdrv_open_child(NULL, options, "file", bs, 
>>> &child_of_bds,
>>> +                               BDRV_CHILD_FILTERED | 
>>> BDRV_CHILD_PRIMARY,
>>> +                               false, errp);
>>> +    if (!bs->file) {
>>>           return -EINVAL;
>>>       }
>>> -    bdrv_ref(source);
>>> -    bs->file = bdrv_attach_child(bs, source, "file", &child_of_bds,
>>> -                                 BDRV_CHILD_FILTERED | 
>>> BDRV_CHILD_PRIMARY,
>>> -                                 errp);
>>> -    if (!bs->file) {
>>> -        error_prepend(errp, "Cannot attach file child: ");
>>> -        bdrv_unref(source);
>>> +    s->target = bdrv_open_child(NULL, options, "target", bs, 
>>> &child_of_bds,
>>> +                                BDRV_CHILD_DATA, false, errp);
>>> +    if (!s->target) {
>>>           return -EINVAL;
>>>       }
>>> @@ -175,7 +168,10 @@ static int cbw_init(BlockDriverState *bs, 
>>> BlockDriverState *source,
>>>               ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | 
>>> BDRV_REQ_NO_FALLBACK) &
>>>                bs->file->bs->supported_zero_flags);
>>> -    s->bcs = block_copy_state_new(bs->file, s->target, false, 
>>> compress, errp);
>>> +    qdict_del(options, "cluster-size");
>>
>> What is this about?
> 
> accidental, will drop. (it's a remaining of my first solution where I 
> tried to pass cluster-size, then I decided that's better move 
> cluster-size detection to block_copy)
> 
>>
>>> +    s->bcs = block_copy_state_new(bs->file, s->target, false,
>>> +            qdict_get_try_bool(options, "x-deprecated-compress", 
>>> false), errp);
>>
>> First, I’d keep the `compress` variable and use it to store the value, 
>> because this doesn’t look very nice.
> 
> OK
> 
>>
>> Second, what’s the story here?  “deprecated” sounds to me like you’re 
>> planning to use a different interface eventually, but looking ahead 
>> for a bit I didn’t find anything yet.
>>
> 
> I should have described it in commit message.
> 
> We have "compress" filter driver. So instead adding "compress" option to 
> every block job or filter, user should use "compress" filter. That's why 
> I don't want to publish compress option for copy-before-write filter. 
> Still we need it to maintain "compress" option of backup job. I also 
> want to deprecate "compress" option in backup, then everything will be 
> clear.

Oh, that’s true.  Until backup’s option is deprecated, I think a comment 
would be more useful than a note in the commit message, though.

Max