5:07 p.m.
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