Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver

On 14.10.2020 14:09, Max Reitz wrote: > On 12.10.20 19:43, 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 name of overlay base node to the copy-on-read >> driver as 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(a)virtuozzo.com&gt; >> --- >>   block/copy-on-read.c | 14 ++++++++++++++ >>   1 file changed, 14 insertions(+) > > Is there a reason why you didn’t add this option to QAPI (as part of a > yet-to-be-created BlockdevOptionsCor)?  Because I’d really like it there. > I agree that passing a base overlay under the base option looks clumsy. We could pass the base node name and find its overlay ourselves here in cor_open(). In that case, we can use the existing QAPI.

The reason I used the existing QAPI is to make it easier for a user to operate with the traditional options and to keep things simple. So, the user shouldn't think what overlay or above-base node to pass.

If we introduce the specific BlockdevOptionsCor, what other options may come with?

>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >> index bcccf0f..c578b1b 100644 >> --- a/block/copy-on-read.c >> +++ b/block/copy-on-read.c >> @@ -24,19 +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 *base_overlay; >>   } BDRVStateCOR; >>       static int cor_open(BlockDriverState *bs, QDict *options, int >> flags, >>                       Error **errp) >>   { >> +    BlockDriverState *base_overlay = NULL; >>       BDRVStateCOR *state = bs->opaque; >> +    /* We need the base overlay node rather than the base itself */ >> +    const char *base_overlay_node = qdict_get_try_str(options, "base"); > > Shouldn’t it be called base-overlay or above-base then? > The base_overlay identifier is used below as the pointer to BS. The base_overlay_node stands for the name of the node. I used that identifier to differ between the types. And the above_base has another meaning per block/stream.c - it can be a temporary filter with a JSON-name.

>>         bs->file = bdrv_open_child(NULL, options, "file", bs, >> &child_of_bds, >>                                  BDRV_CHILD_FILTERED | >> BDRV_CHILD_PRIMARY, >> @@ -52,7 +57,16 @@ 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 (base_overlay_node) { >> +        qdict_del(options, "base"); >> +        base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp); > > I think this is a use-after-free.  The storage @base_overlay_node points > to belongs to a QString, which is referenced only by @options; so > deleting that element of @options should free that string. > > Max > I will swap those two function calls (bdrv_lookup_bs(); qdict_del();). Thank you.

>> +        if (!base_overlay) { >> +            error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node); >> +            return -EINVAL; >> +        } >> +    }