[libvirt] [PATCHv5 02/19] util: storagefile: Introduce universal function to canonicalize paths

[Eric Blake]

On 06/19/2014 07:59 AM, Peter Krempa wrote:
> Introduce a common function that will take a callback to resolve links
> that will be used to canonicalize paths on various storage systems and
> add extensive tests.
> ---
>  src/libvirt_private.syms  |   1 +
>  src/util/virstoragefile.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstoragefile.h |   7 ++
>  tests/virstoragetest.c    | 108 +++++++++++++++++++++++++
>  4 files changed, 311 insertions(+)
> 

> +static int
> +virStorageFileCanonicalizeInjectSymlink(const char *path,
> +                                        size_t at,
> +                                        char ***components,
> +                                        size_t *ncomponents)
> +{
> +    char **tmp = NULL;
> +    char **next;
> +    size_t ntmp = 0;
> +    int ret = -1;
> +
> +    if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp)))
> +        goto cleanup;
> +
> +    /* prepend */
> +    for (next = tmp; *next; next++) {
> +        if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0)
> +            goto cleanup;

Hmm. Laine's VIR_INSERT_ macros have so far only been used to insert one
element at a time, but the underlying functions that the macros call are
capable of inserting an array of elements in one go, which is more
efficient.  I wonder if it's worth adding a macro to expose insertion of
an array, which we could then use here.  But I won't insist - your
series is already long enough that it could be done another day.


> +
> +    TEST_PATH_CANONICALIZE(1, "/", "/");
> +    TEST_PATH_CANONICALIZE(2, "/path", "/path");
> +    TEST_PATH_CANONICALIZE(3, "/path/to/blah", "/path/to/blah");
> +    TEST_PATH_CANONICALIZE(4, "/path/", "/path");
> +    TEST_PATH_CANONICALIZE(5, "///////", "/");
> +    TEST_PATH_CANONICALIZE(6, "//", "//");

Please also test: "///" canonicalizes to "/".

> +    TEST_PATH_CANONICALIZE(7, "", "");
> +    TEST_PATH_CANONICALIZE(8, ".", "");

I'm a bit fuzzy on whether this is a good idea; the empty string is
required to fail with ENOENT, while a lone "." is worth preserving
as-is.  I think test 7 is okay, but if you could tweak the code to make
test 8 return ".", I'd be happier (that is, special case the deletion of
"." to not do that if it is the only remaining element in the array).

> +    TEST_PATH_CANONICALIZE(9, "../", "..");
> +    TEST_PATH_CANONICALIZE(10, "../../", "../..");
> +    TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah");
> +    TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah");
> +    TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah");
> +    TEST_PATH_CANONICALIZE(14, "/././", "/");
> +    TEST_PATH_CANONICALIZE(15, "./././", "");

Another case where I'd be happier if this returns "." (so again, if you
special case that "." is deleted unless it is the last element of the
simplified array)

> +    TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo");
> +    TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah");

ACK if you can make those changes (you may want to post the interdiff)

-- 
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/20140620/c9b96131/attachment-0001.sig>