From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WoYdR-0006j6-As for qemu-devel@nongnu.org; Sun, 25 May 2014 09:38:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WoYdE-0003Xj-VQ for qemu-devel@nongnu.org; Sun, 25 May 2014 09:38:45 -0400 Received: from mail-pb0-x22c.google.com ([2607:f8b0:400e:c01::22c]:53346) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WoYdE-0003XO-FU for qemu-devel@nongnu.org; Sun, 25 May 2014 09:38:32 -0400 Received: by mail-pb0-f44.google.com with SMTP id rq2so6416201pbb.3 for ; Sun, 25 May 2014 06:38:31 -0700 (PDT) Message-ID: <5381F237.4020600@gmail.com> Date: Sun, 25 May 2014 21:37:59 +0800 From: Jun Li MIME-Version: 1.0 References: <1399739757-3111-1-git-send-email-juli@redhat.com> <5370E574.3000503@redhat.com> <53788CA7.6010901@gmail.com> In-Reply-To: <53788CA7.6010901@gmail.com> Content-Type: multipart/alternative; boundary="------------050903000908040903020006" Subject: Re: [Qemu-devel] [PATCH v3] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Jun Li , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, stefanha@redhat.com This is a multi-part message in MIME format. --------------050903000908040903020006 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 >> 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* > -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 >>> --- >>> 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 --------------050903000908040903020006 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit
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
      - 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

    

    

  


--------------050903000908040903020006--