[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] block: for a streaming job, fix relative ba
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [RFC PATCH] block: for a streaming job, fix relative base name arguments |
Date: |
Thu, 12 Apr 2012 10:50:00 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 |
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 <address@hidden>
>
> 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