Re: [PATCH v7 21/47] block: Use CAFs in bdrv_refresh_filename()

[Andrey Shinkevich]

On 15.07.2020 15:52, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
> > bdrv_refresh_filename() and the kind of related bdrv_dirname() should
> > look to the primary child when they wish to copy the underlying file's
> > filename.
> >
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >   block.c | 29 +++++++++++++++++++++--------
> >   1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 8131d0b5eb..7c827fefa0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -6797,6 +6797,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >   {
> >       BlockDriver *drv = bs->drv;
> >       BdrvChild *child;
> > +    BlockDriverState *primary_child_bs;
> >       QDict *opts;
> >       bool backing_overridden;
> >       bool generate_json_filename; /* Whether our default 
> > implementation should
> > @@ -6866,20 +6867,30 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >       qobject_unref(bs->full_open_options);
> >       bs->full_open_options = opts;
> >   +    primary_child_bs = bdrv_primary_bs(bs);
> > +
> >       if (drv->bdrv_refresh_filename) {
> >           /* Obsolete information is of no use here, so drop the old 
> > file name
> >            * information before refreshing it */
> >           bs->exact_filename[0] = '\0';
> >             drv->bdrv_refresh_filename(bs);
> > -    } else if (bs->file) {
> > -        /* Try to reconstruct valid information from the underlying 
> > file */
> > +    } else if (primary_child_bs) {
> > +        /*
> > +         * Try to reconstruct valid information from the underlying
> > +         * file -- this only works for format nodes (filter nodes
> > +         * cannot be probed and as such must be selected by the user
> > +         * either through an options dict, or through a special
> > +         * filename which the filter driver must construct in its
> > +         * .bdrv_refresh_filename() implementation).
> > +         */
>
>
> The caller may not be aware of a filter node and intend to refresh the 
> name of underlying format node.
>
> In that case, the filter driver should redirect the call to the format 
> node.
>
> What are situations the name of the filter itself should be refreshed in?
>
> If there are any, should we do both actions or choose either?
>
> Andrey

I ment the node FILE name.

Andrey


> >             bs->exact_filename[0] = '\0';
> >             /*
> >            * We can use the underlying file's filename if:
> >            * - it has a filename,
> > +         * - the current BDS is not a filter,
>
>
> Should we check the function input parameter for being a filter's BS 
> here, in this function, and handle the case here or let the filter 
> driver function do that or else the caller should check it?
>
> Andrey
>
>
> >            * - the file is a protocol BDS, and
> >            * - opening that file (as this BDS's format) will 
> > automatically create
> >            *   the BDS tree we have right now, that is:
> > @@ -6888,11 +6899,11 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >            *   - no non-file child of this BDS has been overridden by 
> > the user
> >            *   Both of these conditions are represented by 
> > generate_json_filename.
> >            */
> > -        if (bs->file->bs->exact_filename[0] &&
> > -            bs->file->bs->drv->bdrv_file_open &&
> > -            !generate_json_filename)
> > +        if (primary_child_bs->exact_filename[0] &&
> > +            primary_child_bs->drv->bdrv_file_open &&
> > +            !drv->is_filter && !generate_json_filename)
> >           {
> > -            strcpy(bs->exact_filename, bs->file->bs->exact_filename);
> > +            strcpy(bs->exact_filename, 
> > primary_child_bs->exact_filename);
> >           }
> >       }
> >   @@ -6912,6 +6923,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >   char *bdrv_dirname(BlockDriverState *bs, Error **errp)
> >   {
> >       BlockDriver *drv = bs->drv;
> > +    BlockDriverState *child_bs;
> >         if (!drv) {
> >           error_setg(errp, "Node '%s' is ejected", bs->node_name);
> > @@ -6922,8 +6934,9 @@ char *bdrv_dirname(BlockDriverState *bs, Error 
> > **errp)
> >           return drv->bdrv_dirname(bs, errp);
> >       }
> >   -    if (bs->file) {
> > -        return bdrv_dirname(bs->file->bs, errp);
> > +    child_bs = bdrv_primary_bs(bs);
> > +    if (child_bs) {
> > +        return bdrv_dirname(child_bs, errp);
> >       }
> >         bdrv_refresh_filename(bs);