[lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories

[lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories

[Raphaël Beamonte]

David,

Please find here an updated series of patches following your
previous comments. These patches introduce updated versions of the
utils_resolve_relative and utils_expand_path functions, as well as
unit tests to verify their behavior.

Thanks,
Raphaël


Raphaël Beamonte (4):
  Introduce a new utils_resolve_relative function
  Tests: Add test_utils_resolve_relative to unit tests
  Correct the behavior of the utils_expand_path function
  Tests: Add test_utils_expand_path to unit tests

 .gitignore                               |    2 +
 src/common/utils.c                       |  154 +++++++++++++++++++++++----
 src/common/utils.h                       |    1 +
 tests/unit/Makefile.am                   |   20 +++-
 tests/unit/test_utils_expand_path.c      |  170 ++++++++++++++++++++++++++++++
 tests/unit/test_utils_resolve_relative.c |   98 +++++++++++++++++
 tests/unit_tests                         |    2 +
 7 files changed, 421 insertions(+), 26 deletions(-)
 create mode 100644 tests/unit/test_utils_expand_path.c
 create mode 100644 tests/unit/test_utils_resolve_relative.c

-- 
1.7.10.4


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-tools PATCH 1/4] Introduce a new utils_resolve_relative function

[Raphaël Beamonte]

This function aims to resolve relative path such as './' and
'../' in the middle of a path string. This allows to use paths
such as '~/../test' that are received as '/home/x/../test' for
instance.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 src/common/utils.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/common/utils.h |    1 +
 2 files changed, 69 insertions(+)

diff --git a/src/common/utils.c b/src/common/utils.c
index da4c036..4ccf36a 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -36,6 +36,74 @@
 #include "defaults.h"
 
 /*
+ * Resolve the './' and '../' strings in the middle of a path using
+ * our very own way to do it, so that it works even if the directory
+ * does not exist
+ */
+LTTNG_HIDDEN
+char *utils_resolve_relative(const char *path)
+{
+	char *next, *previous, *slash, *start_path, *absolute_path = NULL;
+
+	/* Safety net */
+	if (path == NULL) {
+		goto error;
+	}
+
+	/* Allocate memory for the absolute path */
+	absolute_path = zmalloc(PATH_MAX);
+	if (absolute_path == NULL) {
+		PERROR("zmalloc expand path");
+		goto error;
+	}
+
+	/* Copy the path in the absolute path */
+	strncpy(absolute_path, path, PATH_MAX);
+
+	/* As long as we find '/./' in the path string */
+	while ((next = strstr(absolute_path, "/./"))) {
+
+		/* We prepare the start_path not containing it */
+		start_path = strndup(absolute_path, next - absolute_path);
+
+		/* And we concatenate it with the part after this string */
+		snprintf(absolute_path, PATH_MAX, "%s%s", start_path, next + 2);
+
+		free(start_path);
+	}
+
+	/* As long as we find '/../' in the path string */
+	while ((next = strstr(absolute_path, "/../"))) {
+		/* If the path starts with '/../', there's a problem */
+		if (next == absolute_path) {
+			ERR("%s: Path cannot be resolved", path);
+			goto error;
+		}
+
+		/* We find the last level of directory */
+		previous = absolute_path;
+		while ((slash = strpbrk(previous + 1, "/")) && slash != next) {
+			previous = slash;
+		}
+
+		/* Then we prepare the start_path not containing it */
+		start_path = strndup(absolute_path, previous - absolute_path);
+
+		/* And we concatenate it with the part after the '/../' */
+		snprintf(absolute_path, PATH_MAX, "%s%s", start_path, next + 3);
+
+		free(start_path);
+	}
+
+	return absolute_path;
+
+error:
+	free(absolute_path);
+	return NULL;
+}
+
+
+/*
  * Return the realpath(3) of the path even if the last directory token does not
  * exist. For example, with /tmp/test1/test2, if test2/ does not exist but the
  * /tmp/test1 does, the real path is returned. In normal time, realpath(3)
diff --git a/src/common/utils.h b/src/common/utils.h
index 52f2798..c56942f 100644
--- a/src/common/utils.h
+++ b/src/common/utils.h
@@ -26,6 +26,7 @@
 #define MEBI_LOG2 20
 #define GIBI_LOG2 30
 
+char *utils_resolve_relative(const char *path);
 char *utils_expand_path(const char *path);
 int utils_create_pipe(int *dst);
 int utils_create_pipe_cloexec(int *dst);
-- 
1.7.10.4


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-tools PATCH 2/4] Tests: Add test_utils_resolve_relative to unit tests

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 .gitignore                               |    1 +
 tests/unit/Makefile.am                   |   15 +++--
 tests/unit/test_utils_resolve_relative.c |   98 ++++++++++++++++++++++++++++++
 tests/unit_tests                         |    1 +
 4 files changed, 111 insertions(+), 4 deletions(-)
 create mode 100644 tests/unit/test_utils_resolve_relative.c

diff --git a/.gitignore b/.gitignore
index 5bacea4..2d779e4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -56,6 +56,7 @@ tests/unit/test_session
 tests/unit/test_uri
 tests/unit/test_ust_data
 tests/unit/test_utils_parse_size_suffix
+tests/unit/test_utils_resolve_relative
 kernel_all_events_basic
 kernel_event_basic
 ust_global_event_wildcard
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index a8dec2b..82f6508 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -16,7 +16,8 @@ LIBHASHTABLE=$(top_builddir)/src/common/hashtable/libhashtable.la
 LIBRELAYD=$(top_builddir)/src/common/relayd/librelayd.la
 
 # Define test programs
-noinst_PROGRAMS = test_uri test_session	test_kernel_data test_utils_parse_size_suffix
+noinst_PROGRAMS = test_uri test_session test_kernel_data
+noinst_PROGRAMS += test_utils_parse_size_suffix test_utils_resolve_relative
 
 if HAVE_LIBLTTNG_UST_CTL
 noinst_PROGRAMS += test_ust_data
@@ -82,10 +83,16 @@ test_kernel_data_LDADD = $(LIBTAP) $(LIBCOMMON) $(LIBRELAYD) $(LIBSESSIOND_COMM)
 			 $(LIBHASHTABLE) -lrt
 test_kernel_data_LDADD += $(KERN_DATA_TRACE)
 
-# parse_size_suffix unit test
-UTILS_PARSE_SIZE_SUFFIX=$(top_builddir)/src/common/.libs/utils.o \
+# utils suffix for unit test
+UTILS_SUFFIX=$(top_builddir)/src/common/.libs/utils.o \
 		$(top_builddir)/src/common/.libs/runas.o
 
+# parse_size_suffix unit test
 test_utils_parse_size_suffix_SOURCES = test_utils_parse_size_suffix.c
 test_utils_parse_size_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
-test_utils_parse_size_suffix_LDADD += $(UTILS_PARSE_SIZE_SUFFIX)
+test_utils_parse_size_suffix_LDADD += $(UTILS_SUFFIX)
+
+# resolve_relative unit test
+test_utils_resolve_relative_SOURCES = test_utils_resolve_relative.c
+test_utils_resolve_relative_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
+test_utils_resolve_relative_LDADD += $(UTILS_SUFFIX)
diff --git a/tests/unit/test_utils_resolve_relative.c b/tests/unit/test_utils_resolve_relative.c
new file mode 100644
index 0000000..f43eeff
--- /dev/null
+++ b/tests/unit/test_utils_resolve_relative.c
@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) - 2013 Raphaël Beamonte <raphael.beamonte@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by as
+ * published by the Free Software Foundation; only version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 51
+ * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <tap/tap.h>
+
+#include <src/common/utils.h>
+
+/* For lttngerr.h */
+int lttng_opt_quiet = 1;
+int lttng_opt_verbose = 3;
+
+struct valid_test_input {
+	char *input;
+	char *expected_result;
+};
+
+/* Valid test cases */
+static struct valid_test_input valid_tests_inputs[] = {
+		{ "/a/b/c/d/./e",		"/a/b/c/d/e"	},
+		{ "/a/b/c/d/../e",		"/a/b/c/e"	},
+		{ "/a/b/../c/d/../e",		"/a/c/e"	},
+		{ "/a/b/../../c/./d/./e",	"/c/d/e"	},
+		{ "/a/b/../../c/d/../../e",	"/e"		},
+		{ "/a/b/c/d/../../../../e",	"/e"		},
+		{ "/./a/b/c/d/./e",		"/a/b/c/d/e"	},
+		{ "/",				"/"		},
+		{ "",				""		},
+};
+static const int num_valid_tests =
+		sizeof(valid_tests_inputs) / sizeof(valid_tests_inputs[0]);
+
+/* Invalid test cases */
+static char *invalid_tests_inputs[] = {
+		NULL,
+		"/../a/b/c/d/e",
+		"/a/b/c/d/../../../../../e",
+};
+static const int num_invalid_tests =
+		sizeof(invalid_tests_inputs) / sizeof(invalid_tests_inputs[0]);
+
+static void test_utils_resolve_relative(void)
+{
+	char *result;
+	int i;
+
+	/* Test valid cases */
+	for (i = 0; i < num_valid_tests; i++) {
+		char name[100];
+		sprintf(name, "valid test case: %s", valid_tests_inputs[i].input);
+
+		result = utils_resolve_relative(valid_tests_inputs[i].input);
+		ok(strcmp(result, valid_tests_inputs[i].expected_result) == 0, name);
+
+		free(result);
+	}
+
+	/* Test invalid cases */
+	for (i = 0; i < num_invalid_tests; i++) {
+		char name[100];
+		sprintf(name, "invalid test case: %s", invalid_tests_inputs[i]);
+
+		result = utils_resolve_relative(invalid_tests_inputs[i]);
+		if (result != NULL) {
+			free(result);
+		}
+		ok(result == NULL, name);
+	}
+}
+
+int main(int argc, char **argv)
+{
+	plan_tests(num_valid_tests + num_invalid_tests);
+
+	diag("utils_resolve_relative tests");
+
+	test_utils_resolve_relative();
+
+	return exit_status();
+}
diff --git a/tests/unit_tests b/tests/unit_tests
index 6bf33cc..5d0f291 100644
--- a/tests/unit_tests
+++ b/tests/unit_tests
@@ -3,3 +3,4 @@ unit/test_session
 unit/test_uri
 unit/test_ust_data
 unit/test_utils_parse_size_suffix
+unit/test_utils_resolve_relative
-- 
1.7.10.4


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Raphaël Beamonte]

Even if the utils_expand_path function was intended to allow to
use unexistent directory paths, it was in fact only working for
some kind of arguments. Paths like "foo", "bar/" or "bar/foo"
when the "bar" directory does not exist wasn't working. This
patch introduce a new way to expand paths in this function that
also works properly for these kind of arguments.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 src/common/utils.c |   86 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 22 deletions(-)

diff --git a/src/common/utils.c b/src/common/utils.c
index 4ccf36a..6938a5a 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -112,7 +112,7 @@ error:
 LTTNG_HIDDEN
 char *utils_expand_path(const char *path)
 {
-	const char *end_path = path;
+	const char *end_path = NULL;
 	char *next, *cut_path = NULL, *expanded_path = NULL;
 
 	/* Safety net */
@@ -120,38 +120,80 @@ char *utils_expand_path(const char *path)
 		goto error;
 	}
 
-	/* Find last token delimited by '/' */
-	while ((next = strpbrk(end_path + 1, "/"))) {
-		end_path = next;
-	}
-
-	/* Cut last token from original path */
-	cut_path = strndup(path, end_path - path);
-
+	/* Allocate memory for the expanded path */
 	expanded_path = zmalloc(PATH_MAX);
 	if (expanded_path == NULL) {
 		PERROR("zmalloc expand path");
 		goto error;
 	}
 
-	expanded_path = realpath((char *)cut_path, expanded_path);
-	if (expanded_path == NULL) {
-		switch (errno) {
-		case ENOENT:
-			ERR("%s: No such file or directory", cut_path);
-			break;
-		default:
-			PERROR("realpath utils expand path");
-			break;
+	/* If given path is already absolute */
+	if (*path == '/') {
+		strncpy(expanded_path, path, PATH_MAX);
+	/* Else, we have some work to do */
+	} else {
+		/* Pointer to the last char of the path */
+		const char *last_char = path + strlen(path) - 1;
+
+		end_path = path;
+
+		/* Split part that will be resolved by realpath (relative path from
+		 * current directory using ./ or ../ only) and part that could not
+		 * (directory names)
+		 */
+		while ((next = strpbrk(end_path, "/")) && (next != last_char)) {
+			end_path = next + 1;
+			if (strncmp(end_path, "./", 2) != 0 &&
+					strncmp(end_path, "../", 3) != 0) {
+				break;
+			}
+		}
+
+		/* If this is the end of the string, and we still can resolve it */
+		if (strncmp(end_path, "..\0", 3) == 0 ||
+				strncmp(end_path, ".\0", 2) == 0) {
+			end_path += strlen(end_path);
+		}
+
+		/* If the end part is the whole path, we are in the current dir */
+		if (end_path == path) {
+			cut_path = strdup(".");
+		/* Else, cut the resolvable part from original path */
+		} else {
+			cut_path = strndup(path, end_path - path);
+		}
+
+		/* Resolve the canonical path of the first part of the path */
+		expanded_path = realpath((char *)cut_path, expanded_path);
+		if (expanded_path == NULL) {
+			switch (errno) {
+			case ENOENT:
+				ERR("%s: No such file or directory", cut_path);
+				break;
+			default:
+				PERROR("realpath utils expand path");
+				break;
+			}
+			goto error;
+		}
+
+		/* Add end part to expanded path if not empty */
+		if (*end_path != 0) {
+			strncat(expanded_path, "/", PATH_MAX - strlen(expanded_path) - 1);
+			strncat(expanded_path, end_path,
+					PATH_MAX - strlen(expanded_path) - 1);
 		}
-		goto error;
 	}
 
-	/* Add end part to expanded path */
-	strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
+	/* Resolve the internal './' and '../' strings */
+	next = utils_resolve_relative(expanded_path);
+	if (next == NULL) {
+		goto error;
+	}
 
+	free(expanded_path);
 	free(cut_path);
-	return expanded_path;
+	return next;
 
 error:
 	free(expanded_path);
-- 
1.7.10.4


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-tools PATCH 4/4] Tests: Add test_utils_expand_path to unit tests

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 .gitignore                          |    1 +
 tests/unit/Makefile.am              |    7 +-
 tests/unit/test_utils_expand_path.c |  170 +++++++++++++++++++++++++++++++++++
 tests/unit_tests                    |    1 +
 4 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test_utils_expand_path.c

diff --git a/.gitignore b/.gitignore
index 2d779e4..7041d37 100644
--- a/.gitignore
+++ b/.gitignore
@@ -57,6 +57,7 @@ tests/unit/test_uri
 tests/unit/test_ust_data
 tests/unit/test_utils_parse_size_suffix
 tests/unit/test_utils_resolve_relative
+tests/unit/test_utils_expand_path
 kernel_all_events_basic
 kernel_event_basic
 ust_global_event_wildcard
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 82f6508..fa9c6a8 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -17,7 +17,7 @@ LIBRELAYD=$(top_builddir)/src/common/relayd/librelayd.la
 
 # Define test programs
 noinst_PROGRAMS = test_uri test_session test_kernel_data
-noinst_PROGRAMS += test_utils_parse_size_suffix test_utils_resolve_relative
+noinst_PROGRAMS += test_utils_parse_size_suffix test_utils_resolve_relative test_utils_expand_path
 
 if HAVE_LIBLTTNG_UST_CTL
 noinst_PROGRAMS += test_ust_data
@@ -96,3 +96,8 @@ test_utils_parse_size_suffix_LDADD += $(UTILS_SUFFIX)
 test_utils_resolve_relative_SOURCES = test_utils_resolve_relative.c
 test_utils_resolve_relative_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
 test_utils_resolve_relative_LDADD += $(UTILS_SUFFIX)
+
+# expand_path unit test
+test_utils_expand_path_SOURCES = test_utils_expand_path.c
+test_utils_expand_path_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
+test_utils_expand_path_LDADD += $(UTILS_SUFFIX)
diff --git a/tests/unit/test_utils_expand_path.c b/tests/unit/test_utils_expand_path.c
new file mode 100644
index 0000000..27bfe8f
--- /dev/null
+++ b/tests/unit/test_utils_expand_path.c
@@ -0,0 +1,170 @@
+/*
+ * Copyright (C) - 2013 Raphaël Beamonte <raphael.beamonte@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by as
+ * published by the Free Software Foundation; only version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 51
+ * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <limits.h>
+
+#include <tap/tap.h>
+
+#include <src/common/utils.h>
+
+/* For lttngerr.h */
+int lttng_opt_quiet = 1;
+int lttng_opt_verbose = 3;
+
+struct valid_test_input {
+	char *input;
+	char *relative_part;
+	char *absolute_part;
+};
+
+/* Valid test cases */
+static struct valid_test_input valid_tests_inputs[] = {
+	{ "/a/b/c/d/e",			"",		"/a/b/c/d/e"	},
+	{ "./a/b/c/d/e",		".",		"/a/b/c/d/e"	},
+	{ "../a/b/c/d/../e",		"..",		"/a/b/c/e"	},
+	{ ".././a/b/c/d/./e",		"..",		"/a/b/c/d/e"	},
+	{ "../../a/b/c/d/e",		"../..",	"/a/b/c/d/e"	},
+	{ "./a/b/../c/d/../e",		".",		"/a/c/e"	},
+	{ "../a/b/../../c/./d/./e",	"..",		"/c/d/e"	},
+	{ "../../a/b/../c/d/../../e",	"../..",	"/a/e"		},
+	{ "./a/b/c/d/../../../../e",	".",		"/e"		},
+	{ ".././a/b/c/d/./e",		"..",		"/a/b/c/d/e"	},
+	{ "a/",				".",		"/a/"		},
+	{ "a",				".",		"/a"		},
+	{ "../../",			"../..",	"/"		},
+	{ "../..",			"../..",	""		},
+	{ "../",			"..",		"/"		},
+	{ "..",				"..",		""		},
+	{ "./",				".",		"/"		},
+	{ ".",				".",		""		},
+};
+char **valid_tests_expected_results;
+static const int num_valid_tests =
+		sizeof(valid_tests_inputs) / sizeof(valid_tests_inputs[0]);
+
+/* Invalid test cases */
+static char *invalid_tests_inputs[] = {
+	NULL,
+	"/../a/b/c/d/e",
+	"/a/b/c/d/../../../../../e",
+};
+static const int num_invalid_tests =
+		sizeof(invalid_tests_inputs) / sizeof(invalid_tests_inputs[0]);
+
+int prepare_valid_results()
+{
+	int i;
+	char *relative, *cur_path, *prev_path, *pprev_path, *empty;
+
+	/* Prepare the relative paths */
+	cur_path = realpath(".", NULL);
+	prev_path = realpath("..", NULL);
+	pprev_path = realpath("../..", NULL);
+	empty = strdup("");
+
+	/* allocate memory for the expected results */
+	valid_tests_expected_results = malloc(sizeof(char *) * num_valid_tests);
+	for (i = 0; i < num_valid_tests; i++) {
+		valid_tests_expected_results[i] = malloc(PATH_MAX);
+		if (valid_tests_expected_results[i] == NULL) {
+			fprintf(stderr, "malloc expected results");
+			return 1;
+		}
+
+		if (strcmp(valid_tests_inputs[i].relative_part, ".") == 0) {
+			relative = cur_path;
+		} else if (strcmp(valid_tests_inputs[i].relative_part, "..") == 0) {
+			relative = prev_path;
+		} else if (strcmp(valid_tests_inputs[i].relative_part, "../..") == 0) {
+			relative = pprev_path;
+		} else {
+			relative = empty;
+		}
+
+		snprintf(valid_tests_expected_results[i], PATH_MAX,
+				"%s%s", relative, valid_tests_inputs[i].absolute_part);
+	}
+
+	free(cur_path);
+	free(prev_path);
+	free(pprev_path);
+	free(empty);
+
+	return 0;
+}
+
+int free_valid_results()
+{
+	int i;
+
+	for (i = 0; i < num_valid_tests; i++) {
+		free(valid_tests_expected_results[i]);
+	}
+
+	free(valid_tests_expected_results);
+
+	return 0;
+}
+
+static void test_utils_expand_path(void)
+{
+	char *result;
+	int i;
+
+	/* Test valid cases */
+	for (i = 0; i < num_valid_tests; i++) {
+		char name[100];
+		sprintf(name, "valid test case: %s", valid_tests_inputs[i].input);
+
+		result = utils_expand_path(valid_tests_inputs[i].input);
+		ok(strcmp(result, valid_tests_expected_results[i]) == 0, name);
+
+		free(result);
+	}
+
+	/* Test invalid cases */
+	for (i = 0; i < num_invalid_tests; i++) {
+		char name[100];
+		sprintf(name, "invalid test case: %s", invalid_tests_inputs[i]);
+
+		result = utils_expand_path(invalid_tests_inputs[i]);
+		if (result != NULL) {
+			free(result);
+		}
+		ok(result == NULL, name);
+	}
+}
+
+int main(int argc, char **argv)
+{
+	if (prepare_valid_results() != 0) {
+		return 1;
+	}
+
+	plan_tests(num_valid_tests + num_invalid_tests);
+
+	diag("utils_expand_path tests");
+
+	test_utils_expand_path();
+
+	free_valid_results();
+	return exit_status();
+}
diff --git a/tests/unit_tests b/tests/unit_tests
index 5d0f291..35b31a4 100644
--- a/tests/unit_tests
+++ b/tests/unit_tests
@@ -4,3 +4,4 @@ unit/test_uri
 unit/test_ust_data
 unit/test_utils_parse_size_suffix
 unit/test_utils_resolve_relative
+unit/test_utils_expand_path
-- 
1.7.10.4


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories

[David Goulet]

[-- Attachment #1.1: Type: text/plain, Size: 1324 bytes --]

Merged! Big thanks Raphaël especially for the two unit tests!

David

On 13 Nov (00:34:34), Raphaël Beamonte wrote:
> David,
> 
> Please find here an updated series of patches following your
> previous comments. These patches introduce updated versions of the
> utils_resolve_relative and utils_expand_path functions, as well as
> unit tests to verify their behavior.
> 
> Thanks,
> Raphaël
> 
> 
> Raphaël Beamonte (4):
>   Introduce a new utils_resolve_relative function
>   Tests: Add test_utils_resolve_relative to unit tests
>   Correct the behavior of the utils_expand_path function
>   Tests: Add test_utils_expand_path to unit tests
> 
>  .gitignore                               |    2 +
>  src/common/utils.c                       |  154 +++++++++++++++++++++++----
>  src/common/utils.h                       |    1 +
>  tests/unit/Makefile.am                   |   20 +++-
>  tests/unit/test_utils_expand_path.c      |  170 ++++++++++++++++++++++++++++++
>  tests/unit/test_utils_resolve_relative.c |   98 +++++++++++++++++
>  tests/unit_tests                         |    2 +
>  7 files changed, 421 insertions(+), 26 deletions(-)
>  create mode 100644 tests/unit/test_utils_expand_path.c
>  create mode 100644 tests/unit/test_utils_resolve_relative.c
> 
> -- 
> 1.7.10.4
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Mathieu Desnoyers]

----- Original Message -----
> From: "Raphaël Beamonte" <raphael.beamonte@gmail.com>
> To: dgoulet@efficios.com
> Cc: "Raphaël Beamonte" <raphael.beamonte@gmail.com>, lttng-dev@lists.lttng.org
> Sent: Wednesday, November 13, 2013 12:34:37 AM
> Subject: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the	utils_expand_path function
> 
> Even if the utils_expand_path function was intended to allow to
> use unexistent directory paths, it was in fact only working for
> some kind of arguments. Paths like "foo", "bar/" or "bar/foo"
> when the "bar" directory does not exist wasn't working. This
> patch introduce a new way to expand paths in this function that
> also works properly for these kind of arguments.

I might be missing something, but how about, instead:

1) if path start with /, directly pass it to realpath()
2) if path does not start with /, prepend getcwd() to it, and then call realpath() on the result.

If we can do the same result as this patch without reimplementing tricky string handling, we should really aim for re-using existing libraries.

Thoughts ?

Thanks,

Mathieu

> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  src/common/utils.c |   86
>  ++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 4ccf36a..6938a5a 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -112,7 +112,7 @@ error:
>  LTTNG_HIDDEN
>  char *utils_expand_path(const char *path)
>  {
> -	const char *end_path = path;
> +	const char *end_path = NULL;
>  	char *next, *cut_path = NULL, *expanded_path = NULL;
>  
>  	/* Safety net */
> @@ -120,38 +120,80 @@ char *utils_expand_path(const char *path)
>  		goto error;
>  	}
>  
> -	/* Find last token delimited by '/' */
> -	while ((next = strpbrk(end_path + 1, "/"))) {
> -		end_path = next;
> -	}
> -
> -	/* Cut last token from original path */
> -	cut_path = strndup(path, end_path - path);
> -
> +	/* Allocate memory for the expanded path */
>  	expanded_path = zmalloc(PATH_MAX);
>  	if (expanded_path == NULL) {
>  		PERROR("zmalloc expand path");
>  		goto error;
>  	}
>  
> -	expanded_path = realpath((char *)cut_path, expanded_path);
> -	if (expanded_path == NULL) {
> -		switch (errno) {
> -		case ENOENT:
> -			ERR("%s: No such file or directory", cut_path);
> -			break;
> -		default:
> -			PERROR("realpath utils expand path");
> -			break;
> +	/* If given path is already absolute */
> +	if (*path == '/') {
> +		strncpy(expanded_path, path, PATH_MAX);
> +	/* Else, we have some work to do */
> +	} else {
> +		/* Pointer to the last char of the path */
> +		const char *last_char = path + strlen(path) - 1;
> +
> +		end_path = path;
> +
> +		/* Split part that will be resolved by realpath (relative path from
> +		 * current directory using ./ or ../ only) and part that could not
> +		 * (directory names)
> +		 */
> +		while ((next = strpbrk(end_path, "/")) && (next != last_char)) {
> +			end_path = next + 1;
> +			if (strncmp(end_path, "./", 2) != 0 &&
> +					strncmp(end_path, "../", 3) != 0) {
> +				break;
> +			}
> +		}
> +
> +		/* If this is the end of the string, and we still can resolve it */
> +		if (strncmp(end_path, "..\0", 3) == 0 ||
> +				strncmp(end_path, ".\0", 2) == 0) {
> +			end_path += strlen(end_path);
> +		}
> +
> +		/* If the end part is the whole path, we are in the current dir */
> +		if (end_path == path) {
> +			cut_path = strdup(".");
> +		/* Else, cut the resolvable part from original path */
> +		} else {
> +			cut_path = strndup(path, end_path - path);
> +		}
> +
> +		/* Resolve the canonical path of the first part of the path */
> +		expanded_path = realpath((char *)cut_path, expanded_path);
> +		if (expanded_path == NULL) {
> +			switch (errno) {
> +			case ENOENT:
> +				ERR("%s: No such file or directory", cut_path);
> +				break;
> +			default:
> +				PERROR("realpath utils expand path");
> +				break;
> +			}
> +			goto error;
> +		}
> +
> +		/* Add end part to expanded path if not empty */
> +		if (*end_path != 0) {
> +			strncat(expanded_path, "/", PATH_MAX - strlen(expanded_path) - 1);
> +			strncat(expanded_path, end_path,
> +					PATH_MAX - strlen(expanded_path) - 1);
>  		}
> -		goto error;
>  	}
>  
> -	/* Add end part to expanded path */
> -	strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
> +	/* Resolve the internal './' and '../' strings */
> +	next = utils_resolve_relative(expanded_path);
> +	if (next == NULL) {
> +		goto error;
> +	}
>  
> +	free(expanded_path);
>  	free(cut_path);
> -	return expanded_path;
> +	return next;
>  
>  error:
>  	free(expanded_path);
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Raphaël Beamonte]

> I might be missing something, but how about, instead:
>
> 1) if path start with /, directly pass it to realpath()
> 2) if path does not start with /, prepend getcwd() to it, and then call realpath() on the result.
>
> If we can do the same result as this patch without reimplementing tricky string handling, we should really aim for re-using existing libraries.
>
> Thoughts ?

Hello Mathieu,

Thing is realpath only works if the path exists (realpath(3)). I
perhaps want to give as argument an absolute path that does not exist,
or a path containing strings such as '../' or './'. In such cases,
realpath would do no good, and if we don't accept to use unexisting
paths, then there is no point in using a recursive mkdir.
If fact, if realpath would work in such cases, using an 'if' would not
even be necessary, we could just call realpath and let it resolve the
current directory if necessary.

Hope it helps to understand,

Raphaël

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Mathieu Desnoyers]

----- Original Message -----
> From: "Raphaël Beamonte" <raphael.beamonte@gmail.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "David Goulet" <dgoulet@efficios.com>, lttng-dev@lists.lttng.org
> Sent: Wednesday, November 13, 2013 10:27:03 PM
> Subject: Re: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function
> 
> > I might be missing something, but how about, instead:
> >
> > 1) if path start with /, directly pass it to realpath()
> > 2) if path does not start with /, prepend getcwd() to it, and then call
> > realpath() on the result.
> >
> > If we can do the same result as this patch without reimplementing tricky
> > string handling, we should really aim for re-using existing libraries.
> >
> > Thoughts ?
> 
> Hello Mathieu,
> 
> Thing is realpath only works if the path exists (realpath(3)). I
> perhaps want to give as argument an absolute path that does not exist,
> or a path containing strings such as '../' or './'. In such cases,
> realpath would do no good, and if we don't accept to use unexisting
> paths, then there is no point in using a recursive mkdir.
> If fact, if realpath would work in such cases, using an 'if' would not
> even be necessary, we could just call realpath and let it resolve the
> current directory if necessary.
> 
> Hope it helps to understand,

Yes, it does, thank you!

Mathieu

> 
> Raphaël
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Mathieu Desnoyers]

Here are comments about the patch, even though it has been merged.

----- Original Message -----
> From: "Raphaël Beamonte" <raphael.beamonte@gmail.com>
> To: dgoulet@efficios.com
> Cc: "Raphaël Beamonte" <raphael.beamonte@gmail.com>, lttng-dev@lists.lttng.org
> Sent: Wednesday, November 13, 2013 12:34:37 AM
> Subject: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the	utils_expand_path function
> 
> Even if the utils_expand_path function was intended to allow to
> use unexistent directory paths, it was in fact only working for
> some kind of arguments. Paths like "foo", "bar/" or "bar/foo"
> when the "bar" directory does not exist wasn't working. This
> patch introduce a new way to expand paths in this function that
> also works properly for these kind of arguments.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  src/common/utils.c |   86
>  ++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 4ccf36a..6938a5a 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -112,7 +112,7 @@ error:
>  LTTNG_HIDDEN
>  char *utils_expand_path(const char *path)
>  {
> -	const char *end_path = path;
> +	const char *end_path = NULL;
>  	char *next, *cut_path = NULL, *expanded_path = NULL;
>  
>  	/* Safety net */
> @@ -120,38 +120,80 @@ char *utils_expand_path(const char *path)
>  		goto error;
>  	}
>  
> -	/* Find last token delimited by '/' */
> -	while ((next = strpbrk(end_path + 1, "/"))) {
> -		end_path = next;
> -	}
> -
> -	/* Cut last token from original path */
> -	cut_path = strndup(path, end_path - path);
> -
> +	/* Allocate memory for the expanded path */
>  	expanded_path = zmalloc(PATH_MAX);
>  	if (expanded_path == NULL) {
>  		PERROR("zmalloc expand path");
>  		goto error;
>  	}
>  
> -	expanded_path = realpath((char *)cut_path, expanded_path);
> -	if (expanded_path == NULL) {
> -		switch (errno) {
> -		case ENOENT:
> -			ERR("%s: No such file or directory", cut_path);
> -			break;
> -		default:
> -			PERROR("realpath utils expand path");
> -			break;
> +	/* If given path is already absolute */
> +	if (*path == '/') {
> +		strncpy(expanded_path, path, PATH_MAX);
> +	/* Else, we have some work to do */
> +	} else {
> +		/* Pointer to the last char of the path */
> +		const char *last_char = path + strlen(path) - 1;
> +
> +		end_path = path;
> +
> +		/* Split part that will be resolved by realpath (relative path from

Comments should start with a:

 /*
  * Then start of comment...


> +		 * current directory using ./ or ../ only) and part that could not
> +		 * (directory names)
> +		 */
> +		while ((next = strpbrk(end_path, "/")) && (next != last_char)) {
> +			end_path = next + 1;
> +			if (strncmp(end_path, "./", 2) != 0 &&
> +					strncmp(end_path, "../", 3) != 0) {
> +				break;
> +			}
> +		}
> +
> +		/* If this is the end of the string, and we still can resolve it */
> +		if (strncmp(end_path, "..\0", 3) == 0 ||

There is no point in ending a "" with \0, it implicitly ends with a \0 already.

> +				strncmp(end_path, ".\0", 2) == 0) {

Same here.

> +			end_path += strlen(end_path);
> +		}
> +
> +		/* If the end part is the whole path, we are in the current dir */

Not necessarily. What happens in the unlikely case someone passes a path with simply "/" ?

> +		if (end_path == path) {
> +			cut_path = strdup(".");
> +		/* Else, cut the resolvable part from original path */
> +		} else {
> +			cut_path = strndup(path, end_path - path);
> +		}
> +
> +		/* Resolve the canonical path of the first part of the path */
> +		expanded_path = realpath((char *)cut_path, expanded_path);

So let's say we have:

"../blah/../blah"

so only "../" passed to realpath, or the entire "../blah/../blah" ? (let's suppose blah/ exists), right ?


> +		if (expanded_path == NULL) {
> +			switch (errno) {
> +			case ENOENT:
> +				ERR("%s: No such file or directory", cut_path);
> +				break;
> +			default:
> +				PERROR("realpath utils expand path");
> +				break;
> +			}
> +			goto error;
> +		}
> +
> +		/* Add end part to expanded path if not empty */
> +		if (*end_path != 0) {
> +			strncat(expanded_path, "/", PATH_MAX - strlen(expanded_path) - 1);
> +			strncat(expanded_path, end_path,
> +					PATH_MAX - strlen(expanded_path) - 1);
>  		}
> -		goto error;
>  	}
>  
> -	/* Add end part to expanded path */
> -	strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
> +	/* Resolve the internal './' and '../' strings */
> +	next = utils_resolve_relative(expanded_path);

In the "../blah/../blah" case, here the other ../ would be dealt with by utils_resolve_relative, is that it ?

Thanks,

Mathieu

> +	if (next == NULL) {
> +		goto error;
> +	}
>  
> +	free(expanded_path);
>  	free(cut_path);
> -	return expanded_path;
> +	return next;
>  
>  error:
>  	free(expanded_path);
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 1/4] Introduce a new utils_resolve_relative function

[Mathieu Desnoyers]

Some comments...

----- Original Message -----
> From: "Raphaël Beamonte" <raphael.beamonte@gmail.com>
> To: dgoulet@efficios.com
> Cc: "Raphaël Beamonte" <raphael.beamonte@gmail.com>, lttng-dev@lists.lttng.org
> Sent: Wednesday, November 13, 2013 12:34:35 AM
> Subject: [lttng-dev] [lttng-tools PATCH 1/4] Introduce a new	utils_resolve_relative function
> 
> This function aims to resolve relative path such as './' and
> '../' in the middle of a path string. This allows to use paths
> such as '~/../test' that are received as '/home/x/../test' for
> instance.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  src/common/utils.c |   68
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/common/utils.h |    1 +
>  2 files changed, 69 insertions(+)
> 
> diff --git a/src/common/utils.c b/src/common/utils.c
> index da4c036..4ccf36a 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -36,6 +36,74 @@
>  #include "defaults.h"
>  
>  /*
> + * Resolve the './' and '../' strings in the middle of a path using
> + * our very own way to do it, so that it works even if the directory
> + * does not exist

We should document that this returns allocated memory which needs to be freed by the caller.

Also, we should clearly state that the only reason why we implement this is that realpath() only works on existing paths, which does not cover our use-case of having to resolve partially unexisting paths.

> + */
> +LTTNG_HIDDEN
> +char *utils_resolve_relative(const char *path)
> +{
> +	char *next, *previous, *slash, *start_path, *absolute_path = NULL;
> +
> +	/* Safety net */
> +	if (path == NULL) {
> +		goto error;
> +	}
> +
> +	/* Allocate memory for the absolute path */
> +	absolute_path = zmalloc(PATH_MAX);
> +	if (absolute_path == NULL) {
> +		PERROR("zmalloc expand path");
> +		goto error;
> +	}
> +
> +	/* Copy the path in the absolute path */
> +	strncpy(absolute_path, path, PATH_MAX);
> +
> +	/* As long as we find '/./' in the path string */
> +	while ((next = strstr(absolute_path, "/./"))) {
> +
> +		/* We prepare the start_path not containing it */
> +		start_path = strndup(absolute_path, next - absolute_path);
> +
> +		/* And we concatenate it with the part after this string */
> +		snprintf(absolute_path, PATH_MAX, "%s%s", start_path, next + 2);
> +
> +		free(start_path);
> +	}
> +
> +	/* As long as we find '/../' in the path string */
> +	while ((next = strstr(absolute_path, "/../"))) {
> +		/* If the path starts with '/../', there's a problem */
> +		if (next == absolute_path) {
> +			ERR("%s: Path cannot be resolved", path);
> +			goto error;
> +		}
> +
> +		/* We find the last level of directory */
> +		previous = absolute_path;
> +		while ((slash = strpbrk(previous + 1, "/")) && slash != next) {
> +			previous = slash;
> +		}
> +
> +		/* Then we prepare the start_path not containing it */
> +		start_path = strndup(absolute_path, previous - absolute_path);
> +
> +		/* And we concatenate it with the part after the '/../' */
> +		snprintf(absolute_path, PATH_MAX, "%s%s", start_path, next + 3);
> +
> +		free(start_path);
> +	}

I might be missing something, but how can this work on paths like this ?

"./a/../b/./c/../d/./e" ?

At first glance, it looks like the only cases that work correctly are those in the test case, but there are various variants that won't.

One possible alternative approach to all this path mangling would be to use realpath() on sections of paths (using / as separator, from the start of path), until realpath() fails to lookup the final paths. Maybe then could we simply append the paths as is without resolving. Users would have to know that they should not put "." or ".." in the unexisting part of their paths. Thoughts ?

Thanks,

Mathieu

> +
> +	return absolute_path;
> +
> +error:
> +	free(absolute_path);
> +	return NULL;
> +}
> +
> +
> +/*
>   * Return the realpath(3) of the path even if the last directory token does
>   not
>   * exist. For example, with /tmp/test1/test2, if test2/ does not exist but
>   the
>   * /tmp/test1 does, the real path is returned. In normal time, realpath(3)
> diff --git a/src/common/utils.h b/src/common/utils.h
> index 52f2798..c56942f 100644
> --- a/src/common/utils.h
> +++ b/src/common/utils.h
> @@ -26,6 +26,7 @@
>  #define MEBI_LOG2 20
>  #define GIBI_LOG2 30
>  
> +char *utils_resolve_relative(const char *path);
>  char *utils_expand_path(const char *path);
>  int utils_create_pipe(int *dst);
>  int utils_create_pipe_cloexec(int *dst);
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Raphaël Beamonte]

2013/11/14 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
> Comments should start with a:
>
>  /*
>   * Then start of comment...

Right. Should I submit a new patch for that ?

> There is no point in ending a "" with \0, it implicitly ends with a \0 already.
>
>> +                             strncmp(end_path, ".\0", 2) == 0) {
>
> Same here.

Was done like that for clarity, as "implicit" is not "clear". Would it
be preferable to hide it but still compare the first 2 chars of a 1
char string ? (respectively 3 chars of a 2 chars string)

> Not necessarily. What happens in the unlikely case someone passes a path with simply "/" ?

Then we're not in this arm of the "if". If the path starts by (or is)
'/', it is considered as a "absolute path", and we're just in the arm
of the "if" that starts with the comment "/* If given path is already
absolute */"

> So let's say we have:
>
> "../blah/../blah"
>
> so only "../" passed to realpath, or the entire "../blah/../blah" ? (let's suppose blah/ exists), right ?

Perhaps "blah/" exists, perhaps "blah/" doesn't exist. In both cases,
it should not matter for our function. As we don't have any easy way
to know that directly, we are not considering it for the realpath
call, but only the "relative paths" that we can evaluate ("./" or
"../").
You can look at the loop that starts with the comment "/* Split part
that will be resolved by realpath", and see that in this case, only
"../" will be passed to realpath.

> In the "../blah/../blah" case, here the other ../ would be dealt with by utils_resolve_relative, is that it ?

Exactly, as we can't pass it to realpath in case the directory does not exist.

Raphaël

Re: [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function

[Mathieu Desnoyers]

----- Original Message -----
> From: "Raphaël Beamonte" <raphael.beamonte@gmail.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "David Goulet" <dgoulet@efficios.com>, lttng-dev@lists.lttng.org
> Sent: Thursday, November 14, 2013 11:00:12 AM
> Subject: Re: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function
> 
> 2013/11/14 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
> > Comments should start with a:
> >
> >  /*
> >   * Then start of comment...
> 
> Right. Should I submit a new patch for that ?

A patch that apply on top of lttng-tools master.

> 
> > There is no point in ending a "" with \0, it implicitly ends with a \0
> > already.
> >
> >> +                             strncmp(end_path, ".\0", 2) == 0) {
> >
> > Same here.
> 
> Was done like that for clarity, as "implicit" is not "clear".

"" with \0 is just sign that someone does not understand what he is doing. It's not clearer.

> Would it
> be preferable to hide it but still compare the first 2 chars of a 1
> char string ? (respectively 3 chars of a 2 chars string)

I think you want to keep the comparison with 3 chars, but you might want to test it further to confirm. The comparison between the implied \0 and a character in the other string should do what you are looking for.

> 
> > Not necessarily. What happens in the unlikely case someone passes a path
> > with simply "/" ?
> 
> Then we're not in this arm of the "if". If the path starts by (or is)
> '/', it is considered as a "absolute path", and we're just in the arm
> of the "if" that starts with the comment "/* If given path is already
> absolute */"
> 
> > So let's say we have:
> >
> > "../blah/../blah"
> >
> > so only "../" passed to realpath, or the entire "../blah/../blah" ? (let's
> > suppose blah/ exists), right ?
> 
> Perhaps "blah/" exists, perhaps "blah/" doesn't exist. In both cases,
> it should not matter for our function. As we don't have any easy way
> to know that directly, we are not considering it for the realpath
> call, but only the "relative paths" that we can evaluate ("./" or
> "../").
> You can look at the loop that starts with the comment "/* Split part
> that will be resolved by realpath", and see that in this case, only
> "../" will be passed to realpath.
> 
> > In the "../blah/../blah" case, here the other ../ would be dealt with by
> > utils_resolve_relative, is that it ?
> 
> Exactly, as we can't pass it to realpath in case the directory does not
> exist.

OK. I look forward to seeing your reply for the other patch.

Thanks,

Mathieu

> 
> Raphaël
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-tools PATCH 1/4] Introduce a new utils_resolve_relative function

[Raphaël Beamonte]

2013/11/14 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
> We should document that this returns allocated memory which needs to be freed by the caller.

Right.

> Also, we should clearly state that the only reason why we implement this is that realpath() only works on existing paths, which does not cover our use-case of having to resolve partially unexisting paths.

... or fully unexisting paths ;)
Didn't add the comment here as it is already precised for the
utils_expand_path function that is the one made for that. This
utils_resolve_relative does not aim to replace realpath by itself, but
just take care of resolving relative paths such as "./" and "../". If
we wanted to resolve such paths but not using an absolute path as
argument, it would be useful too.
Yeah, in this very case, it is used by utils_expand_path to replace
realpath. But I think this comment is not appropriate for this
function, that would lead to miscomprehension. Perhaps the comment
about "so that it works even if the directory does not exist" isn't
appropriate either.

> I might be missing something, but how can this work on paths like this ?
>
> "./a/../b/./c/../d/./e" ?

The first while will replace the /./ strings:
- ./a/../b/c/../d/e
The second while will replace the /../ strings:
- ./b/d/e
Just tried to add it in the unit test :
- ok 10 - valid test case: ./a/../b/./c/../d/./e

Yeah, there's still the starting "./", but the starting relative paths
are not in the scope of this function, as it is stated in the
descriptive comment ("in the middle of the path"). It is thus a
working case.

> At first glance, it looks like the only cases that work correctly are those in the test case, but there are various variants that won't.

Can you give me examples ?
I can see one only case that's not working, it's when the path starts
with "../../", as the "/../" will be treated as if it is in the middle
of the string. I'll fix that and send the patch. But if you have any
other examples that would not work, I would gladly use them to verify
that the function works properly, it's sometimes difficult to think
about all the different cases to test (most of my ideas were put in
the unit test).

> One possible alternative approach to all this path mangling would be to use realpath() on sections of paths (using / as separator, from the start of path), until realpath() fails to lookup the final paths. Maybe then could we simply append the paths as is without resolving. Users would have to know that they should not put "." or ".." in the unexisting part of their paths. Thoughts ?

I think that it would be using many calls to realpath for some case of
"try and fail", when we can do something that directly works (one call
to realpath). That's not so many analysis to do on the developer side,
and that's a better experience on the user side. Moreover, this
function does not aim to replace realpath, that's the other one (but
yeah, the other one is using this one).

Thanks,
Raphaël