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

[Jeff Cody]

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 <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.
> 

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