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: 4485 bytes --]


please ignore this one.

On 05/18/2014 06:34 PM, Jun Li wrote:
>
> 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: 11168 bytes --]