Re: [RFC PATCH] block: always update auto_backing_file to full path

[Max Reitz]

On 01.04.21 06:22, Joe Jin wrote:
> Some time after created snapshot, auto_backing_file only has filename,
> this confused overridden check, update it to full path if it is not.
>
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
>   block.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)

Do you have a test for this?

The thing is, I’m not sure about this solution, and I think having a 
test would help me understand better.
bs->auto_backing_file is meant to track what filename a BDS would have 
if we opened bs->backing_file.  To this end, we generally set it to 
whatever bs->backing_file is and then refresh it when we actually do 
open a BDS from it.

So it seems strange to blindly modify it somewhere that doesn’t have to 
do with any of these things.

Now, when opening a backing file from bs->backing_file, we first do make 
it an absolute filename via bdrv_get_full_backing_filename().  So it 
kind of seems prudent to replicate that elsewhere, but I’m not sure 
where exactly.  I would think the best place would be whenever 
auto_backing_file is set to be equal to backing_file (which is generally 
in the image format drivers, when they read the backing file string from 
the image metadata), but that might break the strcmp() in 
bdrv_open_backing_file()...

I don’t think bdrv_refresh_filename() is the right place, though, 
because I’m afraid that this might modify filenames we’ve already 
retrieved from the actual backing BDS, or something.

For example, with this patch applied, iotest 024 fails.

> diff --git a/block.c b/block.c
> index c5b887cec1..8f9a027dee 100644
> --- a/block.c
> +++ b/block.c
> @@ -6969,6 +6969,19 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>           return;
>       }
>   
> +    /* auto_backing_file only has filename, update it to the full path */
> +    if (bs->backing && bs->auto_backing_file[0] != '/') {
> +        char *backing_filename = NULL;
> +        Error *local_err = NULL;
> +
> +        backing_filename = bdrv_make_absolute_filename(bs,
> +                                     bs->auto_backing_file, &local_err);
> +        if (!local_err && backing_filename) {
> +            pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
> +                     backing_filename);
> +            g_free(backing_filename);
> +        }
> +    }

All spaces here are 0xa0 (non-breaking space), which is kind of broken 
and makes it difficult to apply this patch.

Furthermore, if local_err != NULL, we’d need to free it.

Max

>       backing_overridden = bdrv_backing_overridden(bs);
>   
>       if (bs->open_flags & BDRV_O_NO_IO) {