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