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).
+         */
 
         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,
          * - 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);
-- 
2.26.2

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


>   
>           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);

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);
>

On 15.07.20 14: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.

It shouldn’t.  We can only return a plain filename if passing that
filename to qemu (e.g. to -drive) will result in the same block graph
configuration.

This is what the comment means by “filter nodes cannot be probed”: If
there is a filter node, we must generate a json:{} filename, because
otherwise reopening the block device with -drive by passing the filename
generated here would result in a configuration where the filter is missing.

> What are situations the name of the filter itself should be refreshed in?

Hypothetically, if a filename could specify a filter.  For example, say
the filename “filter[copy-on-read]:foo.qcow2” would result in qemu
creating a COR filter on top of a qcow2 node, then we could generate
such a filename.

In practice, filters cannot be configured through plain filenames (apart
from blkverify, but that’s a debugging feature, so it doesn’t really
matter), so there is no such situation.  All filter nodes should have an
empty exact_filename and thus get a json:{} pseudo-filename.

> If there are any, should we do both actions or choose either?
> 
> 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?

bdrv_refresh_filename() is called whenever some node in the block graph
has changed, just to refresh its filename (after that change).  The
caller generally doesn’t really care about the result, so it doesn’t
matter whether the node is a filter or not (i.e., whether it gets a
plain filename or not).

I don’t think the caller should check it, and in this implementation we
simply have to handle filter nodes correctly: That is, not give them a
plain filename.

Max