Re: [Qemu-devel] [RFC PATCH] block: for a streaming job, fix relative base name arguments

[Kevin Wolf]

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