| Message ID | 354a002e4dde2d52c881b794028799d35c5954fb.1334164294.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 11.04.2012 19:29, schrieb Jeff Cody: > When block streaming an image, if a base name is passed in that > is a relative name, but not accessible from the top-level snapshot, > then the relative name is stored incorrectly in the image file. > > For instance, given a snapshot case of: > > /tmp/a/base.raw > /tmp/a/snap1.qcow2 > /tmp/b/snap2.qcow2 > > if they are all chained with relative filenames, like so: > > base(bak:"") <- snap1(bak:"base.raw") <- snap2(bak:"../a/snap1.qcow2") > > Then the merged top-layer will point to an inaccessible path for the > base file: > > base(bak:"") <- snap2(bak:"base.raw") > > This patch checks for a relative path for a basename, and fixes it > so that it is stored in the top-layer image relative to the top-layer > image: > > base(bak:"") <- snap2(bak:"../a/base.raw") > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > I submitted this as an RFC, because I had made a few assumptions that > I would like to get vetted. Assumptions: > > 1.) bs->backing_hd->filename is always the same file as bs->backing_file > 2.) realpath() and dirname() in QEMU behave properly across OS platforms Can you create a qemu-iotests case that is fixed by this patch? It shouldn't be that hard when you base it on 030, which already does some basic streaming testing. > --- > block/stream.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 78 insertions(+), 1 deletions(-) > > diff --git a/block/stream.c b/block/stream.c > index 5c939c7..b82555b 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -13,6 +13,9 @@ > > #include "trace.h" > #include "block_int.h" > +#include <libgen.h> > +#include <string.h> > +#include <limits.h> > > enum { > /* > @@ -163,6 +166,69 @@ static int coroutine_fn is_allocated_base(BlockDriverState *top, > return 1; > } > > +/* Fixes the filename for the proposed backing file, so that > + * A) if it is relative, it points to the relative path accessible > + * from the current bs->filename, OR > + * B) returns 'backing' if 'backing' is already an absolute path Does it really do the latter? If so, why is the path_is_absolute() check in the caller? I think it should be done here, but returning backing itself doesn't sound easy to use. It should rather return a copy the string, so that you know that any result of this function must be freed. (And reading on I see that this is the case and just the comment is inaccurate) > + * > + * Returns NULL if no relative or absolute path can be found. > + */ > +static char *path_find_relative(char *current, char *backing) > +{ > + char *src; > + char *dest; > + char *src_tmp; > + char *src_dir; > + char *rel_backing = NULL; > + char relpath[PATH_MAX] = {0}; > + int offset = 0; > + > + > + src = realpath(current, NULL); My POSIX manpage says: "If resolved_name is a null pointer, the behavior of realpath() is implementation-defined." It seems to have been standardised by now, but I'm not sure if it is a good idea to rely on a quite new feature on some OSes. > + if (src == NULL) { > + goto exit; > + } > + > + dest = realpath(backing, NULL); > + if (dest == NULL) { > + free(src); > + goto exit; Why not initialise src, dest and src_tmp as NULL and goto free_and_exit, which would be the only label? > + } > + > + src_tmp = g_strdup(src); > + > + if (!strcmp(backing, dest)) { > + rel_backing = g_strdup(backing); > + goto free_and_exit; > + } This is a weaker form of path_is_absolute(), right? It only returns backing if the path is absolute and canonical. I don't think we really need the latter condition. > + > + src_dir = dirname(src_tmp); > + g_strlcpy(src_tmp, src_dir, strlen(src)); src_tmp and src_dir may overlap. I believe you get undefined behaviour then. > + > + for (;;) { > + if (!strncmp(src_tmp, dest, strlen(src_tmp))) { strstart()? What happens if I have /tmp/foo/a.img and /tmp/foobar/b.img? The path certainly does start with /tmp/foo, but it's not the same directory. > + offset = strlen(src_tmp); > + offset = strlen(src_tmp) > 1 ? offset+1 : offset; This is a convoluted way of writing if (offset > 1) offset++, right? So I spent quite a while trying to figure out what offset really means. Now I have come to the conclusion that it's the position of the first character in dest that is not in src_tmp? We need a better variable name or a comment here. > + if (offset < strlen(dest)) { > + rel_backing = g_strconcat(relpath, &dest[offset], NULL); > + } If offset >= strlen(dest), then rel_backing is still NULL? Can it happen? Would it be nicer to just return the absolute path then? > + break; > + } else if (strlen(src_tmp) <= 1) { > + break; > + } > + src_dir = dirname(src_tmp); > + g_strlcpy(src_tmp, src_dir, strlen(src)); Same as above. > + g_strlcat(relpath, "../", sizeof(relpath)); > + } > + > +free_and_exit: > + g_free(src_tmp); > + free(src); > + free(dest); > +exit: > + return rel_backing; > +} How about moving the whole function into cutils.c? > + > static void coroutine_fn stream_run(void *opaque) > { > StreamBlockJob *s = opaque; > @@ -172,6 +238,7 @@ static void coroutine_fn stream_run(void *opaque) > int ret = 0; > int n; > void *buf; > + char *base_filename = NULL; > > s->common.len = bdrv_getlength(bs); > if (s->common.len < 0) { > @@ -240,9 +307,19 @@ retry: > if (sector_num == end && ret == 0) { > const char *base_id = NULL; > if (base) { > - base_id = s->backing_file_id; > + /* fix up relative paths, if any */ > + if (!path_is_absolute(s->backing_file_id)) { > + base_filename = path_find_relative(bs->filename, > + s->base->filename); > + base_id = base_filename; > + } else { > + base_id = s->backing_file_id; > + } bdrv_open has the backing file path rewriting code conditional on is_protocol, because it doesn't make much sense there. I guess we should do it here as well. > } > ret = bdrv_change_backing_file(bs, base_id, NULL); > + if (base_filename) { > + g_free(base_filename); > + } The if isn't necessary. Kevin
On 04/12/2012 02:50 AM, Kevin Wolf wrote: >> + >> + src = realpath(current, NULL); > > My POSIX manpage says: > > "If resolved_name is a null pointer, the behavior of realpath() is > implementation-defined." > > It seems to have been standardised by now, but I'm not sure if it is a > good idea to rely on a quite new feature on some OSes. Worse, there are known compliance bugs; realpath() on Solaris 10 will return a relative answer for relative input, for example. I don't know if glib provides a more portable alternative function, but I'd start by searching that.
Il 12/04/2012 14:10, Eric Blake ha scritto: > Worse, there are known compliance bugs; realpath() on Solaris 10 will > return a relative answer for relative input, for example. I don't know > if glib provides a more portable alternative function, but I'd start by > searching that. No, it doesn't. I tried to get one in, but it has been ignored for more than one year now, despite the bug having been open for almost nine years: https://bugzilla.gnome.org/show_bug.cgi?id=111848 Paolo
On Thu, Apr 12, 2012 at 02:13:18PM +0200, Paolo Bonzini wrote: > Il 12/04/2012 14:10, Eric Blake ha scritto: > > Worse, there are known compliance bugs; realpath() on Solaris 10 will > > return a relative answer for relative input, for example. I don't know > > if glib provides a more portable alternative function, but I'd start by > > searching that. > > No, it doesn't. I tried to get one in, but it has been ignored for more > than one year now, despite the bug having been open for almost nine years: > > https://bugzilla.gnome.org/show_bug.cgi?id=111848 Well we can at least take the code from your patch and put it into QEMU, until such time as the BZ is resolved. Regards, Daniel
On 04/12/2012 04:50 AM, Kevin Wolf wrote: > Am 11.04.2012 19:29, schrieb Jeff Cody: >> When block streaming an image, if a base name is passed in that >> is a relative name, but not accessible from the top-level snapshot, >> then the relative name is stored incorrectly in the image file. >> >> For instance, given a snapshot case of: >> >> /tmp/a/base.raw >> /tmp/a/snap1.qcow2 >> /tmp/b/snap2.qcow2 >> >> if they are all chained with relative filenames, like so: >> >> base(bak:"") <- snap1(bak:"base.raw") <- snap2(bak:"../a/snap1.qcow2") >> >> Then the merged top-layer will point to an inaccessible path for the >> base file: >> >> base(bak:"") <- snap2(bak:"base.raw") >> >> This patch checks for a relative path for a basename, and fixes it >> so that it is stored in the top-layer image relative to the top-layer >> image: >> >> base(bak:"") <- snap2(bak:"../a/base.raw") >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> >> I submitted this as an RFC, because I had made a few assumptions that >> I would like to get vetted. Assumptions: >> >> 1.) bs->backing_hd->filename is always the same file as bs->backing_file >> 2.) realpath() and dirname() in QEMU behave properly across OS platforms > > Can you create a qemu-iotests case that is fixed by this patch? It > shouldn't be that hard when you base it on 030, which already does some > basic streaming testing. > Yes, that is a good idea. I'll add that. >> --- >> block/stream.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 78 insertions(+), 1 deletions(-) >> >> diff --git a/block/stream.c b/block/stream.c >> index 5c939c7..b82555b 100644 >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -13,6 +13,9 @@ >> >> #include "trace.h" >> #include "block_int.h" >> +#include <libgen.h> >> +#include <string.h> >> +#include <limits.h> >> >> enum { >> /* >> @@ -163,6 +166,69 @@ static int coroutine_fn is_allocated_base(BlockDriverState *top, >> return 1; >> } >> >> +/* Fixes the filename for the proposed backing file, so that >> + * A) if it is relative, it points to the relative path accessible >> + * from the current bs->filename, OR >> + * B) returns 'backing' if 'backing' is already an absolute path > > Does it really do the latter? If so, why is the path_is_absolute() check > in the caller? > > I think it should be done here, but returning backing itself doesn't > sound easy to use. It should rather return a copy the string, so that > you know that any result of this function must be freed. (And reading on > I see that this is the case and just the comment is inaccurate) > You are right - my comment is inaccurate. It returns a copy of the string. And my comment is further inaccurate - it returns (as you note below) a copy of backing iff it is absolute and canonical. I am leaning towards getting rid of that behaviour entirely. This function would be more useful as a generic function if it just always returns (if possible) a relative path to 'backing' reachable by 'current'. It is easy enough to check for absolute before calling it (which is how it is used now at the end of stream_run(), anyway). I should also note that the return value needs to be freed by the caller. >> + * >> + * Returns NULL if no relative or absolute path can be found. >> + */ >> +static char *path_find_relative(char *current, char *backing) >> +{ >> + char *src; >> + char *dest; >> + char *src_tmp; >> + char *src_dir; >> + char *rel_backing = NULL; >> + char relpath[PATH_MAX] = {0}; >> + int offset = 0; >> + >> + >> + src = realpath(current, NULL); > > My POSIX manpage says: > > "If resolved_name is a null pointer, the behavior of realpath() is > implementation-defined." > > It seems to have been standardised by now, but I'm not sure if it is a > good idea to rely on a quite new feature on some OSes. > As the comments pointed out by Eric and Paolo indicate, the issue is worse on some platforms. It would be nice to be able to rely on canonicalize_file_name(), so I like Daniel's suggestion of pulling in Paolo's proposed glib patch into QEMU. Any objections to bundling Paolo's patch with my next (v1) submission? >> + if (src == NULL) { >> + goto exit; >> + } >> + >> + dest = realpath(backing, NULL); >> + if (dest == NULL) { >> + free(src); >> + goto exit; > > Why not initialise src, dest and src_tmp as NULL and goto free_and_exit, > which would be the only label? > That would be better, agreed. >> + } >> + >> + src_tmp = g_strdup(src); >> + >> + if (!strcmp(backing, dest)) { >> + rel_backing = g_strdup(backing); >> + goto free_and_exit; >> + } > > This is a weaker form of path_is_absolute(), right? It only returns > backing if the path is absolute and canonical. I don't think we really > need the latter condition. > Agree, and I would go further and say we don't need to return if it is absolute. Just let the function do as its name implies, and always return a relative path (except on error, as suggested below). >> + >> + src_dir = dirname(src_tmp); >> + g_strlcpy(src_tmp, src_dir, strlen(src)); > > src_tmp and src_dir may overlap. I believe you get undefined behaviour then. > Ok, right. And dirname() may modify src_tmp, so I can't just use src_tmp. So I may need another intermediate holder. >> + >> + for (;;) { >> + if (!strncmp(src_tmp, dest, strlen(src_tmp))) { > > strstart()? > > What happens if I have /tmp/foo/a.img and /tmp/foobar/b.img? The path > certainly does start with /tmp/foo, but it's not the same directory. > > Yup... this will definitely need to be fixed, thanks. >> + offset = strlen(src_tmp); >> + offset = strlen(src_tmp) > 1 ? offset+1 : offset; > > This is a convoluted way of writing if (offset > 1) offset++, right? > Hmm, indeed correct. Not sure why I did that. I'll make it the non-convoluted way :) > So I spent quite a while trying to figure out what offset really means. > Now I have come to the conclusion that it's the position of the first > character in dest that is not in src_tmp? We need a better variable name > or a comment here. > Correct conclusion... I will try to make that clearer. >> + if (offset < strlen(dest)) { >> + rel_backing = g_strconcat(relpath, &dest[offset], NULL); >> + } > > If offset >= strlen(dest), then rel_backing is still NULL? Can it > happen? Would it be nicer to just return the absolute path then? > I agree, a better default error in all cases would be to return the absolute canonical path over NULL, whenever possible. Not sure if this can happen either, but it won't hurt to check. >> + break; >> + } else if (strlen(src_tmp) <= 1) { >> + break; >> + } >> + src_dir = dirname(src_tmp); >> + g_strlcpy(src_tmp, src_dir, strlen(src)); > > Same as above. > OK. Although, if src_tmp starts out as canonical (src), dirname should never return something longer than src. >> + g_strlcat(relpath, "../", sizeof(relpath)); >> + } >> + >> +free_and_exit: >> + g_free(src_tmp); >> + free(src); >> + free(dest); >> +exit: >> + return rel_backing; >> +} > > How about moving the whole function into cutils.c? > Yes, good idea. >> + >> static void coroutine_fn stream_run(void *opaque) >> { >> StreamBlockJob *s = opaque; >> @@ -172,6 +238,7 @@ static void coroutine_fn stream_run(void *opaque) >> int ret = 0; >> int n; >> void *buf; >> + char *base_filename = NULL; >> >> s->common.len = bdrv_getlength(bs); >> if (s->common.len < 0) { >> @@ -240,9 +307,19 @@ retry: >> if (sector_num == end && ret == 0) { >> const char *base_id = NULL; >> if (base) { >> - base_id = s->backing_file_id; >> + /* fix up relative paths, if any */ >> + if (!path_is_absolute(s->backing_file_id)) { >> + base_filename = path_find_relative(bs->filename, >> + s->base->filename); >> + base_id = base_filename; >> + } else { >> + base_id = s->backing_file_id; >> + } > > bdrv_open has the backing file path rewriting code conditional on > is_protocol, because it doesn't make much sense there. I guess we should > do it here as well. Agree > >> } >> ret = bdrv_change_backing_file(bs, base_id, NULL); >> + if (base_filename) { >> + g_free(base_filename); >> + } > > The if isn't necessary. Agree > > Kevin
Am 12.04.2012 15:17, schrieb Jeff Cody: >>> + * >>> + * Returns NULL if no relative or absolute path can be found. >>> + */ >>> +static char *path_find_relative(char *current, char *backing) >>> +{ >>> + char *src; >>> + char *dest; >>> + char *src_tmp; >>> + char *src_dir; >>> + char *rel_backing = NULL; >>> + char relpath[PATH_MAX] = {0}; >>> + int offset = 0; >>> + >>> + >>> + src = realpath(current, NULL); >> >> My POSIX manpage says: >> >> "If resolved_name is a null pointer, the behavior of realpath() is >> implementation-defined." >> >> It seems to have been standardised by now, but I'm not sure if it is a >> good idea to rely on a quite new feature on some OSes. >> > > As the comments pointed out by Eric and Paolo indicate, the issue is > worse on some platforms. It would be nice to be able to rely on > canonicalize_file_name(), so I like Daniel's suggestion of pulling in > Paolo's proposed glib patch into QEMU. Any objections to bundling > Paolo's patch with my next (v1) submission? I haven't looked at that patch yet, but if it's licensed appropriately (IIUC, Paolo is not the only copyright owner), I think we can pull it in. >>> + } >>> + >>> + src_tmp = g_strdup(src); >>> + >>> + if (!strcmp(backing, dest)) { >>> + rel_backing = g_strdup(backing); >>> + goto free_and_exit; >>> + } >> >> This is a weaker form of path_is_absolute(), right? It only returns >> backing if the path is absolute and canonical. I don't think we really >> need the latter condition. >> > > Agree, and I would go further and say we don't need to return if it is > absolute. Just let the function do as its name implies, and always > return a relative path (except on error, as suggested below). Yes, I think that makes sense. >>> + break; >>> + } else if (strlen(src_tmp) <= 1) { >>> + break; >>> + } >>> + src_dir = dirname(src_tmp); >>> + g_strlcpy(src_tmp, src_dir, strlen(src)); >> >> Same as above. >> > > OK. Although, if src_tmp starts out as canonical (src), dirname should > never return something longer than src. Hm, unexpected answer... Just to make sure we're not talking past each other: I meant the undefined behaviour because src_dir and src_tmp can overlap. Kevin
Il 12/04/2012 15:41, Kevin Wolf ha scritto: >> > >> > As the comments pointed out by Eric and Paolo indicate, the issue is >> > worse on some platforms. It would be nice to be able to rely on >> > canonicalize_file_name(), so I like Daniel's suggestion of pulling in >> > Paolo's proposed glib patch into QEMU. Any objections to bundling >> > Paolo's patch with my next (v1) submission? > I haven't looked at that patch yet, but if it's licensed appropriately > (IIUC, Paolo is not the only copyright owner), I think we can pull it in. I am, but I'm not sure how we would handle backwards-compatibility. IOW, we would carry the patch forever. Paolo
On 04/12/2012 09:41 AM, Kevin Wolf wrote: > Am 12.04.2012 15:17, schrieb Jeff Cody: >>>> + * >>>> + * Returns NULL if no relative or absolute path can be found. >>>> + */ >>>> +static char *path_find_relative(char *current, char *backing) >>>> +{ >>>> + char *src; >>>> + char *dest; >>>> + char *src_tmp; >>>> + char *src_dir; >>>> + char *rel_backing = NULL; >>>> + char relpath[PATH_MAX] = {0}; >>>> + int offset = 0; >>>> + >>>> + >>>> + src = realpath(current, NULL); >>> >>> My POSIX manpage says: >>> >>> "If resolved_name is a null pointer, the behavior of realpath() is >>> implementation-defined." >>> >>> It seems to have been standardised by now, but I'm not sure if it is a >>> good idea to rely on a quite new feature on some OSes. >>> >> >> As the comments pointed out by Eric and Paolo indicate, the issue is >> worse on some platforms. It would be nice to be able to rely on >> canonicalize_file_name(), so I like Daniel's suggestion of pulling in >> Paolo's proposed glib patch into QEMU. Any objections to bundling >> Paolo's patch with my next (v1) submission? > > I haven't looked at that patch yet, but if it's licensed appropriately > (IIUC, Paolo is not the only copyright owner), I think we can pull it in. > >>>> + } >>>> + >>>> + src_tmp = g_strdup(src); >>>> + >>>> + if (!strcmp(backing, dest)) { >>>> + rel_backing = g_strdup(backing); >>>> + goto free_and_exit; >>>> + } >>> >>> This is a weaker form of path_is_absolute(), right? It only returns >>> backing if the path is absolute and canonical. I don't think we really >>> need the latter condition. >>> >> >> Agree, and I would go further and say we don't need to return if it is >> absolute. Just let the function do as its name implies, and always >> return a relative path (except on error, as suggested below). > > Yes, I think that makes sense. > >>>> + break; >>>> + } else if (strlen(src_tmp) <= 1) { >>>> + break; >>>> + } >>>> + src_dir = dirname(src_tmp); >>>> + g_strlcpy(src_tmp, src_dir, strlen(src)); >>> >>> Same as above. >>> >> >> OK. Although, if src_tmp starts out as canonical (src), dirname should >> never return something longer than src. > > Hm, unexpected answer... Just to make sure we're not talking past each > other: I meant the undefined behaviour because src_dir and src_tmp can > overlap. > OK, we were talking past each other. I thought you meant if g_strlcpy() truncated, due to strlen(src_dir) being longer than strlen(src). > Kevin
Am 12.04.2012 16:12, schrieb Paolo Bonzini: > Il 12/04/2012 15:41, Kevin Wolf ha scritto: >>>> >>>> As the comments pointed out by Eric and Paolo indicate, the issue is >>>> worse on some platforms. It would be nice to be able to rely on >>>> canonicalize_file_name(), so I like Daniel's suggestion of pulling in >>>> Paolo's proposed glib patch into QEMU. Any objections to bundling >>>> Paolo's patch with my next (v1) submission? >> I haven't looked at that patch yet, but if it's licensed appropriately >> (IIUC, Paolo is not the only copyright owner), I think we can pull it in. > > I am, but I'm not sure how we would handle backwards-compatibility. > IOW, we would carry the patch forever. Until we decide to have a hard dependency on a glib version that contains the patch, yes. But we carry so many functions of this type, that one more or less shouldn't make a big difference. Kevin
diff --git a/block/stream.c b/block/stream.c index 5c939c7..b82555b 100644 --- a/block/stream.c +++ b/block/stream.c @@ -13,6 +13,9 @@ #include "trace.h" #include "block_int.h" +#include <libgen.h> +#include <string.h> +#include <limits.h> enum { /* @@ -163,6 +166,69 @@ static int coroutine_fn is_allocated_base(BlockDriverState *top, return 1; } +/* Fixes the filename for the proposed backing file, so that + * A) if it is relative, it points to the relative path accessible + * from the current bs->filename, OR + * B) returns 'backing' if 'backing' is already an absolute path + * + * Returns NULL if no relative or absolute path can be found. + */ +static char *path_find_relative(char *current, char *backing) +{ + char *src; + char *dest; + char *src_tmp; + char *src_dir; + char *rel_backing = NULL; + char relpath[PATH_MAX] = {0}; + int offset = 0; + + + src = realpath(current, NULL); + if (src == NULL) { + goto exit; + } + + dest = realpath(backing, NULL); + if (dest == NULL) { + free(src); + goto exit; + } + + src_tmp = g_strdup(src); + + if (!strcmp(backing, dest)) { + rel_backing = g_strdup(backing); + goto free_and_exit; + } + + src_dir = dirname(src_tmp); + g_strlcpy(src_tmp, src_dir, strlen(src)); + + for (;;) { + if (!strncmp(src_tmp, dest, strlen(src_tmp))) { + offset = strlen(src_tmp); + offset = strlen(src_tmp) > 1 ? offset+1 : offset; + if (offset < strlen(dest)) { + rel_backing = g_strconcat(relpath, &dest[offset], NULL); + } + break; + } else if (strlen(src_tmp) <= 1) { + break; + } + src_dir = dirname(src_tmp); + g_strlcpy(src_tmp, src_dir, strlen(src)); + g_strlcat(relpath, "../", sizeof(relpath)); + } + +free_and_exit: + g_free(src_tmp); + free(src); + free(dest); +exit: + return rel_backing; +} + static void coroutine_fn stream_run(void *opaque) { StreamBlockJob *s = opaque; @@ -172,6 +238,7 @@ static void coroutine_fn stream_run(void *opaque) int ret = 0; int n; void *buf; + char *base_filename = NULL; s->common.len = bdrv_getlength(bs); if (s->common.len < 0) { @@ -240,9 +307,19 @@ retry: if (sector_num == end && ret == 0) { const char *base_id = NULL; if (base) { - base_id = s->backing_file_id; + /* fix up relative paths, if any */ + if (!path_is_absolute(s->backing_file_id)) { + base_filename = path_find_relative(bs->filename, + s->base->filename); + base_id = base_filename; + } else { + base_id = s->backing_file_id; + } } ret = bdrv_change_backing_file(bs, base_id, NULL); + if (base_filename) { + g_free(base_filename); + } close_unused_images(bs, base, base_id); }
When block streaming an image, if a base name is passed in that is a relative name, but not accessible from the top-level snapshot, then the relative name is stored incorrectly in the image file. For instance, given a snapshot case of: /tmp/a/base.raw /tmp/a/snap1.qcow2 /tmp/b/snap2.qcow2 if they are all chained with relative filenames, like so: base(bak:"") <- snap1(bak:"base.raw") <- snap2(bak:"../a/snap1.qcow2") Then the merged top-layer will point to an inaccessible path for the base file: base(bak:"") <- snap2(bak:"base.raw") This patch checks for a relative path for a basename, and fixes it so that it is stored in the top-layer image relative to the top-layer image: base(bak:"") <- snap2(bak:"../a/base.raw") Signed-off-by: Jeff Cody <jcody@redhat.com> I submitted this as an RFC, because I had made a few assumptions that I would like to get vetted. Assumptions: 1.) bs->backing_hd->filename is always the same file as bs->backing_file 2.) realpath() and dirname() in QEMU behave properly across OS platforms --- block/stream.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 78 insertions(+), 1 deletions(-)