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

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

[Jun Li]

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.

In this patch, will check the backing_filename is a symlink or not firstly,
then return the full(absolute) path via realpath.

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);
+        }
     }
 }
 
-- 
1.8.3.1

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

[Jun Li]

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.

In this patch, will check the backing_filename is a symlink or not firstly,
then return the full(absolute) path via realpath.

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);
+        }
     }
 }
 
-- 
1.8.3.1

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

[Jun Li]

From: Jun Li <junmuzi@gmail.com>

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.

In this patch, will check the backing_filename is a symlink or not firstly,
then return the full(absolute) path via realpath.

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);
+        }
     }
 }
 
-- 
1.8.3.1

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

[Eric Blake]

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

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.

> 
> 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?

> 
> 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.

> 
> 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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Jun Li]

[-- 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 --]

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

[Jun Li]

[-- 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 --]

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

[Jun Li]

[-- Attachment #1: Type: text/plain, Size: 5530 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?
>


Hi Eric:

I have checked this.

$ grep -wr "path_combine"  ./
./block.c:void path_combine(char *dest, int dest_size,
./block.c:        path_combine(dest, sz, bs->filename, bs->backing_file);
./block.c:            path_combine(filename_tmp, PATH_MAX, 
curr_bs->filename,
./block.c:            path_combine(filename_tmp, PATH_MAX, 
curr_bs->filename,
./block/vmdk.c:        path_combine(extent_path, sizeof(extent_path),
./include/block/block.h:void path_combine(char *dest, int dest_size,


path_combine() is called in above 4 places.

In function bdrv_find_backing_image(), the current realization of 
path_combine() is enough.

             /* If not an absolute filename path, make it relative to 
the current
              * image's filename path */
             path_combine(filename_tmp, PATH_MAX, backing_file);
             ...

             /* We need to make sure the backing filename we are 
comparing against
              * is relative to the current image filename (or absolute) */
             path_combine(filename_tmp, PATH_MAX, curr_bs->backing_file);


In function vmdk_parse_extents(), as only related to current image, so 
no need to change path_combine(), too.

Only bdrv_get_full_backing_filename() need to modify path_combine(). I 
have submit v4.


Best Regards,
Jun Li

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

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

[Stefan Hajnoczi]

On Sun, May 11, 2014 at 12:35:57AM +0800, Jun Li wrote:
> From: Jun Li <junmuzi@gmail.com>
> 
> 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.
> 
> In this patch, will check the backing_filename is a symlink or not firstly,
> then return the full(absolute) path via realpath.
> 
> Signed-off-by: Jun Li <junmuzi@gmail.com>
> ---
>  block.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

Please fix your patch email submission process so it doesn't send the
same patch multiple times.

You've done this several times in the past.  It makes patch review more
difficult than it needs to be for reviewers.  If I have to compare
several emails and figure out which patch is the right one before
reviewing I'm inclined not to review at all.

Thanks,
Stefan

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

[Fam Zheng]

On Wed, 05/14 14:40, Stefan Hajnoczi wrote:
> On Sun, May 11, 2014 at 12:35:57AM +0800, Jun Li wrote:
> > From: Jun Li <junmuzi@gmail.com>
> > 
> > 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.
> > 
> > In this patch, will check the backing_filename is a symlink or not firstly,
> > then return the full(absolute) path via realpath.
> > 
> > Signed-off-by: Jun Li <junmuzi@gmail.com>
> > ---
> >  block.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> Please fix your patch email submission process so it doesn't send the
> same patch multiple times.
> 
> You've done this several times in the past.  It makes patch review more
> difficult than it needs to be for reviewers.  If I have to compare
> several emails and figure out which patch is the right one before
> reviewing I'm inclined not to review at all.
> 

This is indeed very confusing. (I've showed Jun the configuration I have to
send patches.)

If you hit enter too quickly or forgot something, it's OK to try again, but
using distinct subjects is important and makes it easier for us to know what's
happening.

If, by mistake the subjects you send are the same, please reply yourself to the
list, saying "please ignore this one", so we know which to look at.

Fam

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

[Jun Li]

On 05/14/2014 09:05 PM, Fam Zheng wrote:
> On Wed, 05/14 14:40, Stefan Hajnoczi wrote:
>> On Sun, May 11, 2014 at 12:35:57AM +0800, Jun Li wrote:
>>> From: Jun Li <junmuzi@gmail.com>
>>>
>>> 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.
>>>
>>> In this patch, will check the backing_filename is a symlink or not firstly,
>>> then return the full(absolute) path via realpath.
>>>
>>> Signed-off-by: Jun Li <junmuzi@gmail.com>
>>> ---
>>>   block.c | 18 +++++++++++++++++-
>>>   1 file changed, 17 insertions(+), 1 deletion(-)
>> Please fix your patch email submission process so it doesn't send the
>> same patch multiple times.
ok , thx.
>> You've done this several times in the past.  It makes patch review more
>> difficult than it needs to be for reviewers.  If I have to compare
>> several emails and figure out which patch is the right one before
>> reviewing I'm inclined not to review at all.
ok, thank you very much.
>>
> This is indeed very confusing. (I've showed Jun the configuration I have to
> send patches.)
>
> If you hit enter too quickly or forgot something, it's OK to try again, but
> using distinct subjects is important and makes it easier for us to know what's
> happening.
>
> If, by mistake the subjects you send are the same, please reply yourself to the
> list, saying "please ignore this one", so we know which to look at.
ok, got it. Thx.
>
> Fam
Thanks Fam and Stefan, I have configured my msmtp(via gmail) 
successfully. As I specify the wrong smtpserver with smtp.gmail.com 
before， now the gmail smtpserver is smtp.googlemail.com.

Best Regards,
Jun Li