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

[Joe Jin <joe.jin@oracle.com>]

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