Re: [PATCH v10 2/9] copy-on-read: add filter append/drop functions

[Andrey Shinkevich]

On 05.10.2020 16:34, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
> > Provide API for the COR-filter insertion/removal.
> > Also, drop the filter child permissions for an inactive state when the
> > filter node is being removed.
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   block/copy-on-read.c | 84 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block/copy-on-read.h | 35 ++++++++++++++++++++++
> >   2 files changed, 119 insertions(+)
> >   create mode 100644 block/copy-on-read.h
> >
> > diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> > index cb03e0f..3c8231f 100644
> > --- a/block/copy-on-read.c
> > +++ b/block/copy-on-read.c
> > @@ -23,11 +23,21 @@
> >   #include "qemu/osdep.h"
> >   #include "block/block_int.h"
> >   #include "qemu/module.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "block/copy-on-read.h"
> > +
> > +
> > +typedef struct BDRVStateCOR {
> > +    bool active;
> > +} BDRVStateCOR;
> >   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
> >                       Error **errp)
> >   {
> > +    BDRVStateCOR *state = bs->opaque;
> > +
> >       bs->file = bdrv_open_child(NULL, options, "file", bs, 
> > &child_of_bds,
> >                                  BDRV_CHILD_FILTERED | 
> > BDRV_CHILD_PRIMARY,
> >                                  false, errp);
> > @@ -42,6 +52,13 @@ static int cor_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
> >               bs->file->bs->supported_zero_flags);
> > +    state->active = true;
> > +
> > +    /*
> > +     * We don't need to call bdrv_child_refresh_perms() now as the 
> > permissions
> > +     * will be updated later when the filter node gets its parent.
> > +     */
> > +
> >       return 0;
> >   }
> > @@ -57,6 +74,17 @@ static void cor_child_perm(BlockDriverState *bs, 
> > BdrvChild *c,
> >                              uint64_t perm, uint64_t shared,
> >                              uint64_t *nperm, uint64_t *nshared)
> >   {
> > +    BDRVStateCOR *s = bs->opaque;
> > +
> > +    if (!s->active) {
> > +        /*
> > +         * While the filter is being removed
> > +         */
> > +        *nperm = 0;
> > +        *nshared = BLK_PERM_ALL;
> > +        return;
> > +    }
> > +
> >       *nperm = perm & PERM_PASSTHROUGH;
> >       *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
> > @@ -135,6 +163,7 @@ static void cor_lock_medium(BlockDriverState *bs, 
> > bool locked)
> >   static BlockDriver bdrv_copy_on_read = {
> >       .format_name                        = "copy-on-read",
> > +    .instance_size                      = sizeof(BDRVStateCOR),
> >       .bdrv_open                          = cor_open,
> >       .bdrv_child_perm                    = cor_child_perm,
> > @@ -159,4 +188,59 @@ static void bdrv_copy_on_read_init(void)
> >       bdrv_register(&bdrv_copy_on_read);
> >   }
> > +
> > +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> > +                                         QDict *node_options,
> > +                                         int flags, Error **errp)
>
>
> Ok, now function can add ~any filter, not only COR.. But it's a pair for 
> bdrv_cor_filter_drop(), and with "active" hack we don't want make the 
> functions generic I think. So it's OK for now to keep function here and 
> named _cor_.
>
> > +{
> > +    BlockDriverState *cor_filter_bs;
> > +    Error *local_err = NULL;
> > +
> > +    cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
> > +    if (cor_filter_bs == NULL) {
> > +        error_prepend(errp, "Could not create COR-filter node: ");
> > +        return NULL;
> > +    }
>
> You've dropped setting ->implicit field if filter_node_name not 
> specified. Probably caller now can do it.. I don't really care about 
> implicit case, so it's OK for me if it works with iotests.

Thank you for your R-B. The idea behind setting the 'implicit' member by 
a caller is to prepare the code for the node replacement by a function 
at the block generic layer in future. In the scope of this series, that 
may be better to keep it here.

Andrey

> So,
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>