Re: [Qemu-devel] [PATCH v3] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename

[Jun Li <junmuzi@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4264 bytes --]


On 05/12/2014 11:15 PM, Eric Blake wrote:
> On 05/10/2014 10:35 AM, Jun Li wrote:
>> From: Jun Li <junmuzi@gmail.com>
> I see three different "[PATCH v3]" mails with this subject.  To make
> sure we are reviewing the right version, it might help to bump to version 4.
Yes, I have send v4 of this patch and changed the commit description.
>
>> This patch fixed the following bug:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
>>
>> path_combine can not calculate the correct full path name for backing_file.
>> Such as:
>> create a snapshot chain:
>> sn2->sn1->$BASE_IMG
>> backing file is : /home/wookpecker/img.qcow2
>> sn1 : /home/woodpecker/tmp/sn1
>> sn2 : /home/woodpecker/tmp/sn2
>> when create sn2, path_combine can not got a correct path for $BASE_IMG.
> I'm having a hard time parsing that.  We usually represent backing
> chains with the base image on the left:
>
> base <- sn1
>
> Can you show the output of 'qemu-img info --backing-chain
> /home/woodpecker/tmp/sn2' to make it a bit more obvious what your setup
> is?  What command are you issuing that triggers a path_combine that is
> getting the wrong result?
Sorry, this is the wrong commit description, please see it on v4. In 
version 4, I describe it just as followings:
create a snapshot chain:
$BASE_IMG<-sn1<-sn2
backing file is : ./img.qcow2
sn1 : ./tmp/sn1
sn2 : ./tmp/sn2

So,
# /opt/qemu-git-arm/bin/qemu-img info --backing-chain ./tmp/sn2
image: ./tmp/sn2
file format: qcow2
virtual size: 10G (10737418240 bytes)
disk size: 196K
cluster_size: 65536
backing file: ./tmp/sn1 (actual path: /home/lijun/Work/tmp/sn1)
Format specific information:
     compat: 1.1
     lazy refcounts: false

image: /home/lijun/Work/tmp/sn1
file format: qcow2
virtual size: 10G (10737418240 bytes)
disk size: 196K
cluster_size: 65536
backing file: ./img.qcow2 (actual path: /home/lijun/Work/img.qcow2)
Format specific information:
     compat: 1.1
     lazy refcounts: false

image: /home/lijun/Work/img.qcow2
file format: qcow2
virtual size: 10G (10737418240 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
     compat: 1.1
     lazy refcounts: false
------------------
Before this patch,
# qemu-img create -f qcow2 -b ./img.qcow2  ./tmp/sn1
# qemu-img create -f qcow2 -b ./tmp/sn1 ./tmp/sn2
qemu-img: Could not open './tmp/sn1': No such file or directory

>
>> In this patch, will check the backing_filename is a symlink or not firstly,
>> then return the full(absolute) path via realpath.
> This feels fishy to me, and liable to do the wrong thing.  I need more
> context on how to reproduce the issue you are attempting to fix before I
> can even decide if your fix is the right approach.
This patch fixed the following bug(*Bug 1084302* 
<https://bugzilla.redhat.com/show_bug.cgi?id=1084302> -Should improve 
error info when can't find backing file for snapshot):
https://bugzilla.redhat.com/show_bug.cgi?id=1084302
>
>> Signed-off-by: Jun Li <junmuzi@gmail.com>
>> ---
>>   block.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index b749d31..6674719 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -304,10 +304,26 @@ void path_combine(char *dest, int dest_size,
>>   
>>   void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
>>   {
>> +    struct stat sb;
>> +    char *linkname;
>> +
>>       if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) {
>>           pstrcpy(dest, sz, bs->backing_file);
>>       } else {
>> -        path_combine(dest, sz, bs->filename, bs->backing_file);
>> +        if (lstat(bs->backing_file, &sb) == -1) {
>> +            perror("lstat");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        /* Check linkname is a link or not */
>> +        if (S_ISLNK(sb.st_mode)) {
>> +            linkname = malloc(sb.st_size + 1);
>> +            readlink(bs->backing_file, linkname, sb.st_size + 1);
>> +            linkname[sb.st_size] = '\0';
>> +            realpath(linkname, dest);
>> +        } else {
>> +            realpath(bs->backing_file, dest);
>> +        }
> Why are you tweaking just this caller, instead of path_combine() to
> affect all other callers?
I will check. Thx.

Best Regards,
Jun Li

[-- Attachment #2: Type: text/html, Size: 10294 bytes --]