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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list