[libvirt] [PATCHv3 16/36] util: storage: Add helper to resolve relative path difference

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
> This patch introduces a function that will allow us to resolve a
> relative difference between two elements of a disk backing chain. This
> fucntion will be used to allow relative block commit and block pull

s/fucntion/function/

> where we need to specify the new relative name of the image to qemu.
> 
> This patch also adds unit tests for the function to verify that it works
> correctly.
> ---
>  src/libvirt_private.syms  |   1 +
>  src/util/virstoragefile.c |  45 +++++++++++++++++++++
>  src/util/virstoragefile.h |   4 ++
>  tests/virstoragetest.c    | 101 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 151 insertions(+)
> 

> +int
> +virStorageFileGetRelativeBackingPath(virStorageSourcePtr from,
> +                                     virStorageSourcePtr to,
> +                                     char **relpath)

Missing documentation on what this function is intended to do.

> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virStorageSourcePtr next;
> +    char *tmp = NULL;
> +    char ret = -1;
> +
> +    *relpath = NULL;
> +
> +    for (next = from; next; next = next->backingStore) {
> +        if (!next->backingRelative || !next->relPath) {
> +            ret = 1;
> +            goto cleanup;
> +        }
> +
> +        if (next != from)
> +            virBufferAddLit(&buf, "/../");
> +
> +        virBufferAdd(&buf, next->relPath, -1);
> +
> +        if (next == to)
> +            break;
> +    }
> +
> +    if (next != to)
> +        goto cleanup;
> +
> +    if (!(tmp = virBufferContentAndReset(&buf)))
> +        goto cleanup;
> +
> +    if (!(*relpath = virStorageFileSimplifyPath(tmp, true)))

Ouch.  Playing fast and loose with 'path/to/file/../otherfile' as a way
to simplify to 'path/to/otherfile' is very risky.  Instead of doing
'path/to/file'+'/../'+'otherfile', I'd feel better doing
mdirname('path/to/file')+'/'+'otherfile' == 'path/to'+'/'+'otherfile'.
Don't we already have a helper function in util/virfile.h that can
concatenate a relative filename in relation to the containing directory
of another filename?

/me re-reads virFileBuildPath...

nope, not quite what we need.


> 
> +virStorageSource backingchain[9];
> +
> +static void
> +testPathRelativePrepare(void)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < ARRAY_CARDINALITY(backingchain) - 1; i++) {
> +        backingchain[i].backingStore = &backingchain[i+1];
> +    }
> +
> +    backingchain[0].relPath = (char *) "/path/to/some/img";
> +    backingchain[0].backingRelative = false;
> +
> +    backingchain[1].relPath = (char *) "asdf";
> +    backingchain[1].backingRelative = true;
> +
> +    backingchain[2].relPath = (char *) "test";
> +    backingchain[2].backingRelative = true;
> +
> +    backingchain[3].relPath = (char *) "blah";
> +    backingchain[3].backingRelative = true;

1 through 3 are relative to the directory containing img in 0, but that
is '/path/to/some/blah' and not a simplification of
'/path/to/some/img/../blah' since 'img/..' doesn't resolve via POSIX
functions.

> +
> +    backingchain[4].relPath = (char *) "/path/to/some/other/img";
> +    backingchain[4].backingRelative = false;

As a non-relative image, the search for relative starts over again.

> +
> +    backingchain[5].relPath = (char *) "../relative/in/other/path";
> +    backingchain[5].backingRelative = true;

Here, the answer has to be
"/path/to/some/other/../relative/in/other/path", unless we did a stat
test to ensure that '/path/to/some/other/..' resolves to the same
location as '/path/to/some'.

> +
> +    backingchain[6].relPath = (char *) "test";
> +    backingchain[6].backingRelative = true;
> +
> +    backingchain[7].relPath = (char *) "../../../../../below";
> +    backingchain[7].backingRelative = true;

I see that you are trying to test that you can't escape past /...

> +
> +    backingchain[8].relPath = (char *) "a/little/more/upwards";
> +    backingchain[8].backingRelative = true;

but that still doesn't make me feel good about simplifying .. without
actual stat tests.

> +    testPathRelativePrepare();
> +
> +    TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);

I'm trying to wrap my head around this test and the expected results,
and merely got confused.  I need something that describes what we are
doing visually, something like:

Given the chain { "base" <- "intermediate" <- "active" }, we plan to
commit "intermediate" into "base" and need to rewrite the backing file
stored in "active" to point to "base".  TEST_RELATIVE_BACKING(, active,
intermediate, expected) then gives the expected string to write into active.

Except that my formulation doesn't work with what your code expects, or
I'm getting lost somewhere.  backingchain[0] is the active image (living
at "/path/to/some/file", and with backing element currently at the
relative "asdf"); and we are committing backingchain[1] (file at "asdf",
 whose backing is currently "test").  Doesn't that mean that we want to
determine a relative name for "test" that we can store in the metadata
for "/path/to/some/file"?  If so, isn't the answer "asdf", not NULL?

I need more help understanding your goal here.  :(

> +    TEST_RELATIVE_BACKING(2, backingchain[1], backingchain[2], "test");
> +    TEST_RELATIVE_BACKING(3, backingchain[2], backingchain[3], "blah");
> +    TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[3], "blah");
> +    TEST_RELATIVE_BACKING(5, backingchain[1], backingchain[4], NULL);
> +    TEST_RELATIVE_BACKING(6, backingchain[5], backingchain[6], "../relative/in/other/test");
> +    TEST_RELATIVE_BACKING(7, backingchain[5], backingchain[7], "../../../below");
> +    TEST_RELATIVE_BACKING(8, backingchain[5], backingchain[8], "../../../a/little/more/upwards");

and to reiterate, I'm not sure we can elide any 'foo/../' pairs in our
simplification, without actually stat'ing the local filesystem or using
the equivalent libgfapi or other network driver calls.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140602/3bea6cfc/attachment-0001.sig>