[libvirt] [PATCHv3 15/36] util: storagefile: Add helper to resolve "../", "./" and "////" in paths

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
> As gnulib's canonicalize_filename_mode isn't LGPL and relative snapshot
> resolution code will need to clean paths with relative path components

s/components/components,/

> this patch adds a libvirt's own implementation of that functionality and

s/adds a/adds/

> tests to make sure everything works well.
> ---
>  src/util/virstoragefile.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstoragefile.h |  2 +
>  tests/virstoragetest.c    | 83 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+)
> 

> +static char *
> +virStorageFileExportPath(char **components,
> +                         size_t ncomponents,
> +                         bool beginSlash)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    size_t i;
> +    char *ret = NULL;
> +
> +    if (beginSlash)
> +        virBufferAddLit(&buf, "/");
> +
> +    for (i = 0; i < ncomponents; i++) {
> +        if (i != 0)
> +            virBufferAddLit(&buf, "/");

I find it slightly fewer lines of code to just blindly add a trailing
'/' always...

> +
> +        virBufferAdd(&buf, components[i], -1);
> +    }

...then use virBufferTrim() to remove the last unneeded one.  But that's
aesthetic; what you have works.

> +
> +    /* if the output string is empty ... wel just return an empty string */

Did you mean "well" or "we'll"? Either way, it still reads okay (and
shorter) as:

s/wel//

> +virStorageFileSimplifyPath(const char *path,
> +                           bool allow_relative)
> +{
> +    bool beginSlash = false;
> +    char **components = NULL;
> +    size_t ncomponents = 0;
> +    size_t i;
> +    char *ret = NULL;
> +
> +    /* special cases are "" and "//", return them as they are */
> +    if (STREQ(path, "") || STREQ(path, "//")) {
> +        ignore_value(VIR_STRDUP(ret, path));

It's more than just "//" that is special; also special is anything with
a leading double slash ("//foo" should not be simplified to "/foo", even
though "/bar//foo" can be simplified.  Other corner cases: "//foo//bar"
should be simplified to "//foo/bar", "//../blah" can be simplified to
"//blah").  Maybe this means checking if path[0]=='/' && path[1]=='/' &&
path[2]!='/'.

> +++ b/tests/virstoragetest.c
> @@ -512,12 +512,61 @@ testStorageLookup(const void *args)
>      return ret;
>  }
> 
> +
> +struct testPathSimplifyData
> +{
> +    const char *path;
> +    const char *exp_abs;
> +    const char *exp_rel;
> +};
> +

Thanks; adding the test makes it more obvious what the code intends to do :)


>  static int
>  mymain(void)
>  {
>      int ret;
>      virCommandPtr cmd = NULL;
>      struct testChainData data;
> +    struct testPathSimplifyData data3;

data followed by data3 looks a bit odd :)


> +
> +    /* PATH, absolute simplification, relative simplification */
> +    TEST_SIMPLIFY(1, "/", "/", "/");
> +    TEST_SIMPLIFY(2, "/path", "/path", "/path");
> +    TEST_SIMPLIFY(3, "/path/to/blah", "/path/to/blah", "/path/to/blah");
> +    TEST_SIMPLIFY(4, "/path/", "/path", "/path");
> +    TEST_SIMPLIFY(5, "///////", "/", "/");
> +    TEST_SIMPLIFY(6, "//", "//", "//");
> +    TEST_SIMPLIFY(7, "", "", "");
> +    TEST_SIMPLIFY(8, "../", "", "..");
> +    TEST_SIMPLIFY(9, "../../", "", "../..");
> +    TEST_SIMPLIFY(10, "../../blah", "blah", "../../blah");
> +    TEST_SIMPLIFY(11, "/./././blah", "/blah", "/blah");
> +    TEST_SIMPLIFY(12, ".././../././../blah", "blah", "../../../blah");
> +    TEST_SIMPLIFY(13, "/././", "/", "/");
> +    TEST_SIMPLIFY(14, "./././", "", "");

Turning "." into empty looks a bit odd (POSIX requires "" to fail while
"." is the current directory).  Not sure if that needs tweaking.  Also,
maybe it's worth a test of plain "." after test 7 (so that we are
separating test of "." behavior from test 14's coverage of multiple "/."
behavior).

> +    TEST_SIMPLIFY(15, "blah/../foo", "foo", "foo");

This is not always true. It is wrong if blah/ is not a (symlink to a)
directory; and if blah IS a symlink, it can still be wrong if it is a
symlink to somewhere else in the hierarchy.  While I'm in favor of
simplifying /./ and a//b, I'm less certain about the benefits of
reducing '..' without actually stat'ing the underlying filesystem.

> +    TEST_SIMPLIFY(16, "foo/bar/../blah", "foo/blah", "foo/blah");
> +    TEST_SIMPLIFY(17, "foo/bar/.././blah", "foo/blah", "foo/blah");
> +    TEST_SIMPLIFY(18, "/path/to/foo/bar/../../../../../../../../baz", "/baz", "/baz");
> +    TEST_SIMPLIFY(19, "path/to/foo/bar/../../../../../../../../baz", "baz", "../../../../baz");
> +    TEST_SIMPLIFY(20, "path/to/foo/bar", "path/to/foo/bar", "path/to/foo/bar");
> +    TEST_SIMPLIFY(21, "some/path/to/image.qcow/../image2.qcow/../image3.qcow/",
> +                  "some/path/to/image3.qcow", "some/path/to/image3.qcow");

Yep, the more I look at this, the more I worry that simplifying '..' is
wrong. :(

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