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

[Vladimir Sementsov-Ogievskiy]

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