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(+)

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);
+        }
+    }
     backing_overridden = bdrv_backing_overridden(bs);
 
     if (bs->open_flags & BDRV_O_NO_IO) {
-- 
2.21.1 (Apple Git-122.3)

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

Hi Max,

Thanks very much for your replies.

On 4/1/21 2:57 AM, Max Reitz wrote:
> 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?
My issue is my guest image is on NFS, I could created snapshot from ovirt but I could not delete it, tried to commit it by virsh and it complained qemu internal error:
# virsh blockcommit snap-test sda --active --verbose --pivot
error: internal error: qemu block name 'json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/919a4cda-bf11-4bb7-a2e3-d9e4515ed646"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/24d28fcd-67e3-49d2-8860-2110f3a5e796"}}' doesn't match expected '/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/24d28fcd-67e3-49d2-8860-2110f3a5e796'
Tracked qemu block and I found when issue happened, bdrv_backing_overridden() was tried to compare absolute path(bs->backing->bs->filename) with relative path(bs->auto_backing_file) but they are same filename, then it triggered generated json filename.
Regarding auto_backing_file, it updated by qcow2_do_open(), and read the backing file name from image:
    /* read the backing file name */                                                      
    if (header.backing_file_offset != 0) {                                                
        len = header.backing_file_size;                                                   
        if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||              
            len >= sizeof(bs->backing_file)) {                                            
            error_setg(errp, "Backing file name too long");                               
            ret = -EINVAL;                                                                
            goto fail;                                                                    
        }                                                                                 
        ret = bdrv_pread(bs->file, header.backing_file_offset,                            
                         bs->auto_backing_file, len);                                     
        if (ret < 0) {                                                                    
            error_setg_errno(errp, -ret, "Could not read backing file name");             
            goto fail;                                                                    
        }                                                                                 
        bs->auto_backing_file[len] = '\0';                                                
        pstrcpy(bs->backing_file, sizeof(bs->backing_file),                               
                bs->auto_backing_file);                                                   
        s->image_backing_file = g_strdup(bs->auto_backing_file);                          
    }                                                                                     

Updated auto_backing_file to absolute path, I could successfully delete snapshot from ovirt, or execute blockcommit by virsh.

>
> 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.
Yes after applied my patch, I can reproduced the failure, it caused by bdrv_make_absolute_filename() returned relative path, not sure if this is bdrv_make_absolute_filename() bug?
Added path_is_absolute(backing_filename) check before update auto_backing_file, test passed:
+        backing_filename = bdrv_make_absolute_filename(bs, bs->auto_backing_file, &local_err);
+        if (!local_err && backing_filename && path_is_absolute(backing_filename)) {
+            pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+                     backing_filename);

>
>> 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.
Sorry about this, I may use git send-email to send it.

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

Thanks for reminder, will take care of this.

Thanks,
Joe
>
> Max
>
>>       backing_overridden = bdrv_backing_overridden(bs);
>>         if (bs->open_flags & BDRV_O_NO_IO) {
>>
>