Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> We are going to use the COR-filter for a block-stream job.
> To limit COR operations by the base node in the backing chain during
> stream job, pass the bottom node name, that is the first non-filter
> overlay of the base, to the copy-on-read driver as the base node itself
> may change due to possible concurrent jobs.
> The rest of the functionality will be implemented in the patch that
> follows.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/copy-on-read.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 618c4c4..3d8e4db 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -24,18 +24,24 @@
>   #include "block/block_int.h"
>   #include "qemu/module.h"
>   #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qdict.h"
>   #include "block/copy-on-read.h"
>   
>   
>   typedef struct BDRVStateCOR {
>       bool active;
> +    BlockDriverState *bottom_bs;
>   } BDRVStateCOR;
>   
>   
>   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>                       Error **errp)
>   {
> +    BlockDriverState *bottom_bs = NULL;
>       BDRVStateCOR *state = bs->opaque;
> +    /* Find a bottom node name, if any */
> +    const char *bottom_node = qdict_get_try_str(options, "bottom");
>   
>       bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>                                  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> @@ -51,7 +57,17 @@ 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);
>   
> +    if (bottom_node) {
> +        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
> +        if (!bottom_bs) {
> +            error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);

QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h that 
such macros should not be used in new code. And don't forget to drop qerror.h 
include line.

> +            qdict_del(options, "bottom");

this may be moved above "bottom_bs = ..", to not call it after "if" in separate.

> +            return -EINVAL;
> +        }
> +        qdict_del(options, "bottom");
> +    }
>       state->active = true;
> +    state->bottom_bs = bottom_bs;
>   
>       /*
>        * We don't need to call bdrv_child_refresh_perms() now as the permissions


--
Best regards,
Vladimir