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

[Joe Jin]

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