snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename

PATCH In review Jun Li 5 participants 8 messages May 10, 2014 – May 25, 2014

lore

Share & tools

lore

Jun Li

author

2014-05-10 16:36 UTC #0 lore raw

PATCH v3 1/1 snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename

From: Jun Li <[email protected]>

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

---
 block.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

block.c +17 -1

index b749d31..6674719 100644
@@ -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);
+        }
     }
 }

Signature · 1 line

1.8.3.1

Eric Blake

2014-05-12 15:15 UTC #1 lore raw

Quoted 1 line · Jun Li

From: Jun Li <[email protected]>

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.

Quoted 4 lines

This patch fixed the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .

Quoted 8 lines

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?

Quoted 3 lines

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.

Quoted 25 lines

Signed-off-by Jun Li <[email protected]>

---
 block.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

block.c +7 -1

index b749d31..6674719 100644
@@ -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);
+        }

block.c void path_combine(char *dest, int dest_size, · line 326

Quoted 10 lines

block.c +10 -0

+
+        /* 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?

Signature · 2 lines

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

Jun Li

2014-05-18 10:35 UTC #2 lore raw

Quoted 4 lines · Eric Blake

Quoted 1 line · Jun Li
From: Jun Li <[email protected]>
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.

Quoted 15 lines

Quoted 4 lines
This patch fixed the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
Quoted 8 lines
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:

Quoted 6 lines

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

Quoted 6 lines

Quoted 3 lines
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

Quoted 29 lines

Signed-off-by Jun Li <[email protected]>

---
  block.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

block.c +11 -1

index b749d31..6674719 100644
@@ -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);

Quoted 8 lines

block.c void path_combine(char *dest, int dest_size, · line 326
Quoted 6 lines
block.c +6 -0
+            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

Jun Li

2014-05-25 13:38 UTC #3 lore raw

please ignore this one.

Quoted 117 lines · Jun Li

Quoted 4 lines · Eric Blake
Quoted 1 line · Jun Li
From: Jun Li<[email protected]>
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.
Quoted 14 lines
Quoted 11 lines
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:
Quoted 6 lines
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
Quoted 5 lines
Quoted 2 lines
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
Quoted 28 lines
Quoted 28 lines
Signed-off-by Jun Li<[email protected]>
---
  block.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)
block.c +11 -1
index b749d31..6674719 100644
@@ -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);
Quoted 8 lines
block.c void path_combine(char *dest, int dest_size, · line 326
Quoted 6 lines
block.c +6 -0
+            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

Jun Li

2014-05-25 13:35 UTC #4 lore raw

In reply to Eric Blake

Quoted 4 lines · Eric Blake

Quoted 1 line · Jun Li
From: Jun Li <[email protected]>
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.

Quoted 15 lines

Quoted 4 lines
This patch fixed the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
Quoted 8 lines
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:

Quoted 6 lines

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?

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

# 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

Quoted 6 lines

Quoted 3 lines
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.

Quoted 29 lines

Signed-off-by Jun Li <[email protected]>

---
  block.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

block.c +11 -1

index b749d31..6674719 100644
@@ -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);

Quoted 9 lines

block.c void path_combine(char *dest, int dest_size, · line 326
Quoted 6 lines
block.c +6 -0
+            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

Stefan Hajnoczi

2014-05-14 12:41 UTC #5 lore raw

In reply to Jun Li

Quoted 14 lines · Jun Li

From: Jun Li <[email protected]>
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.

Quoted 7 lines

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

Fam Zheng

2014-05-14 13:05 UTC #6 lore raw

Quoted 23 lines · Stefan Hajnoczi

Quoted 14 lines · Jun Li
From: Jun Li <[email protected]>
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.
Quoted 7 lines
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 <[email protected]>
---
 block.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Quoted 8 lines

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

Jun Li

2014-05-17 12:37 UTC #7 lore raw

Quoted 19 lines · Fam Zheng

Quoted 18 lines · Stefan Hajnoczi
Quoted 17 lines · Jun Li
From: Jun Li <[email protected]>
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.

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
Signed-off-by Jun Li <[email protected]>
---
  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.

Quoted 4 lines

Quoted 4 lines
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.

Quoted 10 lines

Quoted 1 line
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.

Quoted 2 lines

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