snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename
PATCH 1/1 snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename
When bs->filename and bs->backing_file are relative pathname and not under the same directory, path_combine() can not give the correct path for bs->backing_file. So add get_localfile_absolute_path to get absolute path for local file.
e.g:
$ pwd /tmp $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./test 5M Formatting './test', fmt=qcow2 size=5242880 encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test1 -b ./test Formatting './tmp/test1', fmt=qcow2 size=5242880 backing_file='./test' encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test2 -b ./tmp/test1 qemu-img: ./tmp/test2: Could not open './tmp/test1': Could not open backing file: Could not open './tmp/./test': No such file or directory: No such file or directory
This patch also fixes the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2
--- block.c | 28 +++++++++++++++++++++++++++- include/block/block.h | 2 ++ 2 files changed, 29 insertions(+), 1 deletion(-)
block.c +27 -1
@@ -259,6 +259,32 @@ int path_is_absolute(const char *path)#endif } +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename) +{ +struct stat sb; +char *linkname; + +if (path_is_absolute (filename)) { +pstrcpy (dest, dest_size, filename); + }else { +if (lstat (filename, &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 (filename, linkname, sb.st_size +1 ); + linkname[sb.st_size] ='\0' ; +realpath (linkname, dest); + }else { +realpath (filename, dest); + } + } +} +/* if filename is absolute, just copy it to dest. Otherwise, build a path to it by considering it is relative to base_path. URL are supported. */
@@ -308,7 +334,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)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); +get_localfile_absolute_path (dest, sz, bs->backing_file); } }
include/block/block.h +2 -0
@@ -398,6 +398,8 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,int bdrv_is_snapshot (BlockDriverState *bs);int path_is_absolute (const char *path); +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename);void path_combine (char *dest,int dest_size,const char *base_path,const char *filename);
Signature · 1 line
1.9.3
Quoted 4 lines · Jun Li
When
bs->filenameandbs->backing_fileare relative pathname and not under the same directory,path_combine()can not give the correct path forbs->backing_file. So addget_localfile_absolute_pathto get absolute path for local file.
Well, for me it is the correct path. I'm using
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b backing.qcow2 foo/backed.qcow2
I guess this patch will break that?
I know this isn't ideal, but that's just how it works, so we would break existing usage.
Furthermore, compiling with this patch fails for me because on my system readlink() and realpath() have the attribute warn_unused_result. Also, I'm not sure, but readlink() may only resolve one link (which may point to another link in turn).
And finally, with this patch applied and the return value issue fixed:
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b foo/backing.qcow2 foo/backed.qcow2 64M $ cd foo $ qemu-img info backed.qcow2 lstat: No such file or directory
Because the backing field in the qcow2 file points to "foo/backing.qcow2" which does not exist.
If you are giving a relative filename to the "-b" option during qemu-img create, this filename is not (contrary to intuition) relative to the current working directory of qemu-img, but it is the value which is put directly into the backing field of the file to be created; and that value is relative to that file's directory.
See also http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00632.html and http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00633.html
Max
Quoted 82 lines
e.g:
$ pwd /tmp $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./test 5M Formatting './test', fmt=qcow2 size=5242880 encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test1 -b ./test Formatting './tmp/test1', fmt=qcow2 size=5242880 backing_file='./test' encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test2 -b ./tmp/test1 qemu-img: ./tmp/test2: Could not open './tmp/test1': Could not open backing file: Could not open './tmp/./test': No such file or directory: No such file or directoryThis patch also fixes the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2Signed-off-by Jun Li <[email protected]>--- block.c | 28 +++++++++++++++++++++++++++- include/block/block.h | 2 ++ 2 files changed, 29 insertions(+), 1 deletion(-)block.c +27
-1@@ -259,6 +259,32 @@ int path_is_absolute(const char *path)#endif } +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename) +{ +struct stat sb; +char *linkname; + +if (path_is_absolute (filename)) { +pstrcpy (dest, dest_size, filename); + }else { +if (lstat (filename, &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 (filename, linkname, sb.st_size +1 ); + linkname[sb.st_size] ='\0' ; +realpath (linkname, dest); + }else { +realpath (filename, dest); + } + } +} +/* if filename is absolute, just copy it to dest. Otherwise, build a path to it by considering it is relative to base_path. URL are supported. */ @@ -308,7 +334,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)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); +get_localfile_absolute_path (dest, sz, bs->backing_file); } }include/block/block.h +2
-0@@ -398,6 +398,8 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,int bdrv_is_snapshot (BlockDriverState *bs);int path_is_absolute (const char *path); +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename);void path_combine (char *dest,int dest_size,const char *base_path,const char *filename);
Quoted 40 lines · Max Reitz
Quoted 4 lines · Jun Li
When
bs->filenameandbs->backing_fileare relative pathname and not under the same directory,path_combine()can not give the correct path forbs->backing_file. So addget_localfile_absolute_pathto get absolute path for local file.Well, for me it is the correct path. I'm using
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b backing.qcow2 foo/backed.qcow2I guess this patch will break that?
I know this isn't ideal, but that's just how it works, so we would break existing usage.
Furthermore, compiling with this patch fails for me because on my system
readlink()andrealpath()have the attributewarn_unused_result. Also, I'm not sure, butreadlink()may only resolve one link (which may point to another link in turn).And finally, with this patch applied and the return value issue fixed:
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b foo/backing.qcow2 foo/backed.qcow2 64M $ cd foo $ qemu-img info backed.qcow2 lstat: No such file or directoryBecause the backing field in the qcow2 file points to "foo/backing.qcow2" which does not exist.
If you are giving a relative filename to the "-b" option during qemu-img create, this filename is not (contrary to intuition) relative to the current working directory of qemu-img, but it is the value which is put directly into the backing field of the file to be created; and that value is relative to that file's directory.
Quoted 4 lines
See also
http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00632.html and
http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00633.html
Hi Max,
Do above two patches can resolve this bz https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2 ? If it can solve this bz, so please ignore my patch.
If not, why not using absolute filename to instead relative filename to the "-b" option during qemu-img create? Using absolute filename just need more spaces for backing_file of structure BlockDriverState.
BTW, the above two patches are very necessary, I think.
Regards,
Jun Li
Quoted 81 lines
Quoted 80 lines
e.g:
$ pwd /tmp $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./test 5M Formatting './test', fmt=qcow2 size=5242880 encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test1 -b ./test Formatting './tmp/test1', fmt=qcow2 size=5242880 backing_file='./test' encryption=off cluster_size=65536 lazy_refcounts=off $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test2 -b ./tmp/test1 qemu-img: ./tmp/test2: Could not open './tmp/test1': Could not open backing file: Could not open './tmp/./test': No such file or directory: No such file or directoryThis patch also fixes the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2Signed-off-by Jun Li <[email protected]>--- block.c | 28 +++++++++++++++++++++++++++- include/block/block.h | 2 ++ 2 files changed, 29 insertions(+), 1 deletion(-)block.c +27
-1@@ -259,6 +259,32 @@ int path_is_absolute(const char *path)#endif } +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename) +{ +struct stat sb; +char *linkname; + +if (path_is_absolute (filename)) { +pstrcpy (dest, dest_size, filename); + }else { +if (lstat (filename, &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 (filename, linkname, sb.st_size +1 ); + linkname[sb.st_size] ='\0' ; +realpath (linkname, dest); + }else { +realpath (filename, dest); + } + } +} +/* if filename is absolute, just copy it to dest. Otherwise, build a path to it by considering it is relative to base_path. URL are supported. */ @@ -308,7 +334,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)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); +get_localfile_absolute_path (dest, sz, bs->backing_file); } }include/block/block.h +2
-0@@ -398,6 +398,8 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,int bdrv_is_snapshot (BlockDriverState *bs);int path_is_absolute (const char *path); +void get_localfile_absolute_path (char *dest,int dest_size, +const char *filename);void path_combine (char *dest,int dest_size,const char *base_path,const char *filename);
Quoted 40 lines · Jun Li
Quoted 39 lines · Max Reitz
Quoted 4 lines · Jun Li
When
bs->filenameandbs->backing_fileare relative pathname and not under the same directory,path_combine()can not give the correct path forbs->backing_file. So addget_localfile_absolute_pathto get absolute path for local file.Well, for me it is the correct path. I'm using
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b backing.qcow2 foo/backed.qcow2I guess this patch will break that?
I know this isn't ideal, but that's just how it works, so we would break existing usage.
Furthermore, compiling with this patch fails for me because on my system
readlink()andrealpath()have the attributewarn_unused_result. Also, I'm not sure, butreadlink()may only resolve one link (which may point to another link in turn).And finally, with this patch applied and the return value issue fixed:
$ mkdir -p foo $ qemu-img create -f qcow2 foo/backing.qcow2 64M $ qemu-img create -f qcow2 -b foo/backing.qcow2 foo/backed.qcow2 64M $ cd foo $ qemu-img info backed.qcow2 lstat: No such file or directoryBecause the backing field in the qcow2 file points to "foo/backing.qcow2" which does not exist.
If you are giving a relative filename to the "-b" option during qemu-img create, this filename is not (contrary to intuition) relative to the current working directory of qemu-img, but it is the value which is put directly into the backing field of the file to be created; and that value is relative to that file's directory.
Quoted 7 lines
Quoted 4 lines
See also
http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00632.html and
http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00633.htmlHi Max,
Do above two patches can resolve this bz https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2 ?
No, I don't think so.
However, the real reason for the problem you're describing in that comment 2 is probably that the blockdev-snapshot-sync implementation directly uses the filename of the BDS to be synced instead of first making it an absolute filename or at least ensuring that it is an absolute filename. To do this, the bdrv_img_create() call in external_snapshot_prepare() in blockdev.c probably should not give state->old_bs->filename directly as the backing filename, but make it absolute first (by using realpath(), I guess).
We should not bother to try to find an equivalent relative filename to the target file.
Quoted 5 lines
If it can solve this bz, so please ignore my patch.
If not, why not using absolute filename to instead relative filename to the "-b" option during qemu-img create? Using absolute filename just need more spaces for
backing_fileof structure BlockDriverState.
And it doesn't work anymore if I want to have a relative filename.
Suppose I want to have, for whatever reason, some image, and two other images which use that image as a backing file. Further suppose that I want to be able to distribute these three images in an archive. Without using a relative filename for the backing file, that won't work.
I personally want to be able to use relative backing filenames and easiest way to do that is by directly storing the string given to qemu-img create for the "-b" option in the backing filename field of the file to be created. This is not very intuitive, as I said before, because that path is therefore relative to the directory of the new file and not relative to qemu-img's working directory; but changing this behavior would break compatibility and in my opinion no real gain (other than being able to use tab completion, but I remember there are some git commands which always require a pass relative to the repository's root directory, independently of where your current working directory is; so qemu-img is at least not the only program which does not always interpret a relative filename relatively to its current working directory).
A solution to this problem may be to always try to open the backing file in bdrv_img_create (not only when the size is omitted) so that the user immediately sees that the path given is wrong. However, I can imagine some users actually wanting to reference a non-existing backing file and creating that file later on, so we probably don't want that either.
I'd just leave image creation as it is, and for the above bug report (for the problem described in your comment 2) fix external_snapshot_prepare() in blockdev.c
Max