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