|
From: | Jun Li |
Subject: | Re: [Qemu-devel] [PATCH v3] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename |
Date: | Sun, 25 May 2014 21:35:14 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 05/12/2014 11:15 PM, Eric Blake
wrote:
On 05/10/2014 10:35 AM, Jun Li wrote:From: Jun Li <address@hidden>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 <address@hidden> --- 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 |
[Prev in Thread] | Current Thread | [Next in Thread] |