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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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.

> 
>> +    qdict_del(options, "x-deprecated-compress");
>>       if (!s->bcs) {
>>           error_prepend(errp, "Cannot create block-copy-state: ");
>>           return -EINVAL;
>> @@ -212,6 +208,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
>>       int ret;
>>       BDRVCopyBeforeWriteState *state;
>>       BlockDriverState *top;
>> +    QDict *opts;
>>       assert(source->total_sectors == target->total_sectors);
>> @@ -223,7 +220,13 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
>>       }
>>       state = top->opaque;
>> -    ret = cbw_init(top, source, target, compress, errp);
>> +    opts = qdict_new();
>> +    qdict_put_str(opts, "file", bdrv_get_node_name(source));
>> +    qdict_put_str(opts, "target", bdrv_get_node_name(target));
>> +    qdict_put_bool(opts, "x-deprecated-compress", compress);
>> +
>> +    ret = cbw_init(top, opts, errp);
>> +    qobject_unref(opts);
>>       if (ret < 0) {
>>           goto fail;
>>       }
>>
> 


-- 
Best regards,
Vladimir