[lttng-dev] [lttng-tools PATCH 0/2] utils_expand_path as realpath(3) for non-existing directories

[lttng-dev] [lttng-tools PATCH 0/2] utils_expand_path as realpath(3) for non-existing directories

[Raphaël Beamonte]

The utils_expand_path function of lttng-tools, in src/common/utils.c,
was meant to replace the realpath(3) function of glibc in cases we
want to allow non-existing paths, such as specifying the output directory
for a trace.
While using the lttng-tools command line to create some traces, I found
out that some use cases weren't working. For instance, using paths like
"foo", "bar/" or "bar/foo" when the "bar" directory does not exist was
not working.

This series of two (2) patches aims to correct the behavior of the
utils_expand_path function, introducing a new utils_resolve_relative
function meant to resolve relative paths ("./" and "../") in the middle
of a path string.


Rapha?l Beamonte (2):
  Introduce a new utils_resolve_relative function
  Correct the behavior of the utils_expand_path function

 src/common/utils.c |  120 +++++++++++++++++++++++++++++++++++++++++++---------
 src/common/utils.h |    1 +
 2 files changed, 100 insertions(+), 21 deletions(-)

-- 
1.7.10.4

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

[Raphaël Beamonte]

This functions allows to resolve relative path such as './'
and '../' inside 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 at gmail.com>
---
 src/common/utils.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/common/utils.h |    1 +
 2 files changed, 49 insertions(+)

diff --git a/src/common/utils.c b/src/common/utils.c
index da4c036..7b52760 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -36,6 +36,54 @@
 #include "defaults.h"
 
 /*
+ * Resolve the './' and '../' strings inside a path using our
+ * very own way to do it, so that it works even if the directory
+ * does not exist
+ */
+LTTNG_HIDDEN
+int utils_resolve_relative(char *path)
+{
+	char *next, *previous, *slash, *start_path;
+
+	/* As long as we find '/./' in the path string */
+	while ((next = strstr(path, "/./"))) {
+
+		/* We prepare the start_path not containing it */
+		start_path = strndup(path, next - path);
+
+		/* And we concatenate it with the part after this string */
+		snprintf(path, PATH_MAX, "%s%s", start_path, next + 2);
+
+		free(start_path);
+	}
+
+	while ((next = strstr(path, "/../"))) {
+		/* If the path starts with '/../', there's a problem */
+		if (next == path) {
+			ERR("%s: Path cannot be resolved", path);
+			return 1;
+		}
+
+		/* We find the last level of directory */
+		previous = path;
+		while ((slash = strpbrk(previous + 1, "/")) && slash != next) {
+			previous = slash;
+		}
+
+		/* Then we prepare the start_path not containing it */
+		start_path = strndup(path, previous - path);
+
+		/* And we concatenate it with the part after the '/../' */
+		snprintf(path, PATH_MAX, "%s%s", start_path, next + 3);
+
+		free(start_path);
+	}
+
+	return 0;
+}
+
+
+/*
  * 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..ced3584 100644
--- a/src/common/utils.h
+++ b/src/common/utils.h
@@ -26,6 +26,7 @@
 #define MEBI_LOG2 20
 #define GIBI_LOG2 30
 
+int utils_resolve_relative(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] [lttng-tools PATCH 1/2] Introduce a new utils_resolve_relative function

[David Goulet]

On 07 Nov (23:32:13), Rapha?l Beamonte wrote:
> This functions allows to resolve relative path such as './'
> and '../' inside 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 at gmail.com>
> ---
>  src/common/utils.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/common/utils.h |    1 +
>  2 files changed, 49 insertions(+)
> 
> diff --git a/src/common/utils.c b/src/common/utils.c
> index da4c036..7b52760 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -36,6 +36,54 @@
>  #include "defaults.h"
>  
>  /*
> + * Resolve the './' and '../' strings inside a path using our
> + * very own way to do it, so that it works even if the directory
> + * does not exist
> + */
> +LTTNG_HIDDEN
> +int utils_resolve_relative(char *path)
> +{
> +	char *next, *previous, *slash, *start_path;
> +
> +	/* As long as we find '/./' in the path string */
> +	while ((next = strstr(path, "/./"))) {
> +
> +		/* We prepare the start_path not containing it */
> +		start_path = strndup(path, next - path);
> +
> +		/* And we concatenate it with the part after this string */
> +		snprintf(path, PATH_MAX, "%s%s", start_path, next + 2);
> +
> +		free(start_path);
> +	}
> +
> +	while ((next = strstr(path, "/../"))) {
> +		/* If the path starts with '/../', there's a problem */
> +		if (next == path) {
> +			ERR("%s: Path cannot be resolved", path);
> +			return 1;
> +		}
> +
> +		/* We find the last level of directory */
> +		previous = path;
> +		while ((slash = strpbrk(previous + 1, "/")) && slash != next) {
> +			previous = slash;
> +		}
> +
> +		/* Then we prepare the start_path not containing it */
> +		start_path = strndup(path, previous - path);
> +
> +		/* And we concatenate it with the part after the '/../' */
> +		snprintf(path, PATH_MAX, "%s%s", start_path, next + 3);

I'm not to keen with modifying directly the given path. I would really
prefer having this function return a newly allocated char * and taking a
const char *.  We could want to keep the path given for whatever reason
and also you have no idea if the path given is NULL terminated or not
nor its size.

Also, a small unit test testing this function and the other one in the
next patch would be awesome if you have 2 minutes for that. Those
functions are not that trivial to follow for a human :P, there is a lot
of string mangling and it's getting confusing after a while.

Thanks!
David

> +
> +		free(start_path);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/*
>   * 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..ced3584 100644
> --- a/src/common/utils.h
> +++ b/src/common/utils.h
> @@ -26,6 +26,7 @@
>  #define MEBI_LOG2 20
>  #define GIBI_LOG2 30
>  
> +int utils_resolve_relative(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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: Digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20131112/fc8ff39a/attachment-0001.pgp>

[lttng-dev] [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] [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 at 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] [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 at efficios.com
> Cc: "Rapha?l Beamonte" <raphael.beamonte at gmail.com>, lttng-dev at 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 at 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 at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

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

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

[Raphaël Beamonte]

2013/11/14 Mathieu Desnoyers <mathieu.desnoyers at 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

[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 at 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 at 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] [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 at 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] [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 at efficios.com
> Cc: "Rapha?l Beamonte" <raphael.beamonte at gmail.com>, lttng-dev at 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 at 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 at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

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

[lttng-dev] [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

[lttng-dev] [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 at efficios.com>
> Cc: "David Goulet" <dgoulet at efficios.com>, lttng-dev at 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] [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 at efficios.com
> Cc: "Rapha?l Beamonte" <raphael.beamonte at gmail.com>, lttng-dev at 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 at 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 at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

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

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

[Raphaël Beamonte]

2013/11/14 Mathieu Desnoyers <mathieu.desnoyers at 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

[lttng-dev] [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 at efficios.com>
> Cc: "David Goulet" <dgoulet at efficios.com>, lttng-dev at 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 at 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] [lttng-tools PATCH 0/5] Follow up to the realpath(3) for unexistent paths

[Raphaël Beamonte]

David,

Following the discussions we had on IRC with Mathieu and the last
mail exchange, here is a series of patch that should answer to our
enquiries.

The first two patches are linked to the utils_expand_path unit test
and correct its behavior and the expected results (some input was
at first considered invalid, but during our discussions it was
mentionned that it should be considered valid).

The next three ones introduce a new utils_partial_realpath that,
as its name says it, allows to partially resolve a real path for
a given path. This function is then used in utils_expand_path to
resolve the paths, and the utils_resolve_relative function is
removed as it is not necessary anymore in this new "setup".

Waiting for your comments,
Rapha?l

Rapha?l Beamonte (5):
  Tests: add a check in test_utils_expand_path to avoid segfault if
    result is NULL
  Tests: move invalid tests in test_utils_expand_path that should be
    valid
  Introduce a new utils_partial_realpath function
  Change the utils_expand_path function to use utils_partial_realpath
  Remove the utils_resolve_relative function that is not useful anymore

 .gitignore                               |    1 -
 src/common/utils.c                       |  256 ++++++++++++++++++------------
 src/common/utils.h                       |    3 +-
 tests/unit/Makefile.am                   |    7 +-
 tests/unit/test_utils_expand_path.c      |    7 +-
 tests/unit/test_utils_resolve_relative.c |   98 ------------
 tests/unit_tests                         |    1 -
 7 files changed, 158 insertions(+), 215 deletions(-)
 delete mode 100644 tests/unit/test_utils_resolve_relative.c

-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 1/5] Tests: add a check in test_utils_expand_path to avoid segfault if result is NULL

[Raphaël Beamonte]

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 tests/unit/test_utils_expand_path.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/unit/test_utils_expand_path.c b/tests/unit/test_utils_expand_path.c
index 27bfe8f..b79744b 100644
--- a/tests/unit/test_utils_expand_path.c
+++ b/tests/unit/test_utils_expand_path.c
@@ -135,7 +135,8 @@ static void test_utils_expand_path(void)
 		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);
+		ok(result != NULL &&
+				strcmp(result, valid_tests_expected_results[i]) == 0, name);
 
 		free(result);
 	}
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 2/5] Tests: move invalid tests in test_utils_expand_path that should be valid

[Raphaël Beamonte]

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 tests/unit/test_utils_expand_path.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test_utils_expand_path.c b/tests/unit/test_utils_expand_path.c
index b79744b..bb460a2 100644
--- a/tests/unit/test_utils_expand_path.c
+++ b/tests/unit/test_utils_expand_path.c
@@ -55,6 +55,8 @@ static struct valid_test_input valid_tests_inputs[] = {
 	{ "..",				"..",		""		},
 	{ "./",				".",		"/"		},
 	{ ".",				".",		""		},
+	{ "/../a/b/c/d/e",		"",		"/a/b/c/d/e"	},
+	{ "/a/b/c/d/../../../../../e",	"",		"/e"		},
 };
 char **valid_tests_expected_results;
 static const int num_valid_tests =
@@ -63,8 +65,6 @@ static const int num_valid_tests =
 /* 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]);
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 3/5] Introduce a new utils_partial_realpath function

[Raphaël Beamonte]

This new utils function allows to resolve partially the paths
using realpath. As realpath(3) is not available to use for
unexistent paths, this function that allows to resolve partially
existent paths can be used in cases we don't know if the path
fully exists. It first resolves the existent part using
realpath(3) and then concatenate the unexistent part to it.

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 src/common/utils.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/common/utils.h |    2 +
 2 files changed, 130 insertions(+)

diff --git a/src/common/utils.c b/src/common/utils.c
index 6938a5a..57add31 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -36,6 +36,134 @@
 #include "defaults.h"
 
 /*
+ * Return a partial realpath(3) of the path even if the full path does not
+ * exist. For instance, with /tmp/test1/test2/test3, if test2/ does not exist
+ * but the /tmp/test1 does, the real path for /tmp/test1 is concatened with
+ * /test2/test3 then returned. In normal time, realpath(3) fails if the end
+ * point directory does not exist.
+ * In case resolved_path is NULL, the string returned was allocated in the
+ * function and thus need to be freed by the caller. The size argument allows
+ * to specify the size of the resolved_path argument if given, or the size to
+ * allocate.
+ */
+LTTNG_HIDDEN
+char *utils_partial_realpath(const char *path, char *resolved_path, size_t size)
+{
+	char *cut_path, *try_path = NULL, *try_path_prev = NULL;
+	const char *next, *prev, *end;
+
+	/* Safety net */
+	if (path == NULL) {
+		goto error;
+	}
+
+	/*
+	 * Identify the end of the path, we don't want to treat the
+	 * last char if it is a '/', we will just keep it on the side
+	 * to be added at the end, and return a value coherent with
+	 * the path given as argument
+	 */
+	end = path + strlen(path);
+	if (*(end-1) == '/') {
+		end--;
+	}
+
+	/* Initiate the values of the pointers before looping */
+	next = path;
+	prev = next;
+	/* Only to ensure try_path is not NULL to enter the while */
+	try_path = (char *)next;
+
+	/* Resolve the canonical path of the first part of the path */
+	while (try_path != NULL && next != end) {
+		/*
+		 * If there is not any '/' left, we want to try with
+		 * the full path
+		 */
+		next = strpbrk(next + 1, "/");
+		if (next == NULL) {
+			next = end;
+		}
+
+		/* Cut the part we will be trying to resolve */
+		cut_path = strndup(path, next - path);
+
+		/* Try to resolve this part */
+		try_path = realpath((char *)cut_path, NULL);
+		if (try_path == NULL) {
+			/*
+			 * There was an error, we just want to be assured it
+			 * is linked to an unexistent directory, if it's another
+			 * reason, we spawn an error
+			 */
+			switch (errno) {
+			case ENOENT:
+				/* Ignore the error */
+				break;
+			default:
+				PERROR("realpath (partial_realpath)");
+				goto error;
+				break;
+			}
+		} else {
+			/* Save the place we are before trying the next step */
+			free(try_path_prev);
+			try_path_prev = try_path;
+			prev = next;
+		}
+
+		/* Free the allocated memory */
+		free(cut_path);
+	};
+
+	/* Allocate memory for the resolved path if necessary */
+	if (resolved_path == NULL) {
+		resolved_path = zmalloc(size);
+		if (resolved_path == NULL) {
+			PERROR("zmalloc resolved path");
+			goto error;
+		}
+	}
+
+	/*
+	 * If we were able to solve at least partially the path, we can concatenate
+	 * what worked and what didn't work
+	 */
+	if (try_path_prev != NULL) {
+		/* If we risk to concatenate two '/', we remove one of them */
+		if (try_path_prev[strlen(try_path_prev) - 1] == '/' && prev[0] == '/') {
+			try_path_prev[strlen(try_path_prev) - 1] = '\0';
+		}
+
+		/*
+		 * Duplicate the memory used by prev in case resolved_path and
+		 * path are pointers for the same memory space
+		 */
+		cut_path = strdup(prev);
+
+		/* Concatenate the strings */
+		snprintf(resolved_path, size, "%s%s", try_path_prev, cut_path);
+
+		/* Free the allocated memory */
+		free(cut_path);
+		free(try_path_prev);
+	/*
+	 * Else, we just copy the path in our resolved_path to
+	 * return it as is
+	 */
+	} else {
+		strncpy(resolved_path, path, size);
+	}
+
+	/* Then we return the 'partially' resolved path */
+	return resolved_path;
+
+error:
+	free(resolved_path);
+	return NULL;
+}
+
+/*
  * 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
diff --git a/src/common/utils.h b/src/common/utils.h
index c56942f..036e416 100644
--- a/src/common/utils.h
+++ b/src/common/utils.h
@@ -27,6 +27,8 @@
 #define GIBI_LOG2 30
 
 char *utils_resolve_relative(const char *path);
+char *utils_partial_realpath(const char *path, char *resolved_path,
+		size_t size);
 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] [lttng-tools PATCH 4/5] Change the utils_expand_path function to use utils_partial_realpath

[Raphaël Beamonte]

As most of the resolve-related work can now be done using
utils_partial_realpath, the utils_expand_path function can
call it and concentrate on resolving the relative paths in
the middle of a path string, such as '/./' and '/../'.

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 src/common/utils.c |  121 +++++++++++++++++++++++-----------------------------
 1 file changed, 53 insertions(+), 68 deletions(-)

diff --git a/src/common/utils.c b/src/common/utils.c
index 57add31..d28e313 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -230,102 +230,87 @@ error:
 	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)
- * fails if the end point directory does not exist.
+ * Make a full resolution of the given path even if it doesn't exist.
+ * This function uses the utils_partial_realpath function to resolve
+ * symlinks and relatives paths at the start of the string, and
+ * implements functionnalities to resolve the './' and '../' strings
+ * in the middle of a path. This function is only necessary because
+ * realpath(3) does not accept to resolve unexistent paths.
+ * The returned string was allocated in the function, it is thus of
+ * the responsibility of the caller to free this memory.
  */
 LTTNG_HIDDEN
 char *utils_expand_path(const char *path)
 {
-	const char *end_path = NULL;
-	char *next, *cut_path = NULL, *expanded_path = NULL;
+	char *next, *previous, *slash, *start_path, *absolute_path = NULL;
 
 	/* Safety net */
 	if (path == NULL) {
 		goto error;
 	}
 
-	/* Allocate memory for the expanded path */
-	expanded_path = zmalloc(PATH_MAX);
-	if (expanded_path == NULL) {
+	/* Allocate memory for the absolute_path */
+	absolute_path = zmalloc(PATH_MAX);
+	if (absolute_path == NULL) {
 		PERROR("zmalloc expand path");
 		goto error;
 	}
 
-	/* If given path is already absolute */
-	if (*path == '/') {
-		strncpy(expanded_path, path, PATH_MAX);
-	/* Else, we have some work to do */
+	/*
+	 * If the path is not already absolute nor explicitly relative,
+	 * consider we're in the current directory
+	 */
+	if (*path != '/' && strncmp(path, "./", 2) != 0 &&
+			strncmp(path, "../", 3) != 0) {
+		snprintf(absolute_path, PATH_MAX, "./%s", path);
+		/* Else, we just copy the path */
 	} else {
-		/* Pointer to the last char of the path */
-		const char *last_char = path + strlen(path) - 1;
+		strncpy(absolute_path, path, PATH_MAX);
+	}
 
-		end_path = path;
+	/* Resolve partially our path */
+	absolute_path = utils_partial_realpath(absolute_path,
+			absolute_path, PATH_MAX);
 
-		/* 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;
-			}
-		}
+	/* As long as we find '/./' in the working_path string */
+	while ((next = strstr(absolute_path, "/./"))) {
 
-		/* 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);
-		}
+		/* We prepare the start_path not containing it */
+		start_path = strndup(absolute_path, next - absolute_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);
-		}
+		/* And we concatenate it with the part after this string */
+		snprintf(absolute_path, PATH_MAX, "%s%s", start_path, next + 2);
 
-		/* 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;
-		}
+		free(start_path);
+	}
 
-		/* 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);
+	/* As long as we find '/../' in the working_path string */
+	while ((next = strstr(absolute_path, "/../"))) {
+		/* We find the last level of directory */
+		previous = absolute_path;
+		while ((slash = strpbrk(previous, "/")) && slash != next) {
+			previous = slash + 1;
 		}
-	}
 
-	/* Resolve the internal './' and '../' strings */
-	next = utils_resolve_relative(expanded_path);
-	if (next == NULL) {
-		goto error;
+		/* 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 + 4);
+
+		/* We can free the memory used for the start path*/
+		free(start_path);
+
+		/* Then we verify for symlinks using partial_realpath */
+		absolute_path = utils_partial_realpath(absolute_path,
+				absolute_path, PATH_MAX);
 	}
 
-	free(expanded_path);
-	free(cut_path);
-	return next;
+	return absolute_path;
 
 error:
-	free(expanded_path);
-	free(cut_path);
+	free(absolute_path);
 	return NULL;
 }
 
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 5/5] Remove the utils_resolve_relative function that is not useful anymore

[Raphaël Beamonte]

As all of the work is now done in utils_partial_realpath and
utils_expand_path, utils_resolve_relative is not necessary
anymore and should be deleted from the sources.

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 .gitignore                               |    1 -
 src/common/utils.c                       |   67 --------------------
 src/common/utils.h                       |    1 -
 tests/unit/Makefile.am                   |    7 +--
 tests/unit/test_utils_resolve_relative.c |   98 ------------------------------
 tests/unit_tests                         |    1 -
 6 files changed, 1 insertion(+), 174 deletions(-)
 delete mode 100644 tests/unit/test_utils_resolve_relative.c

diff --git a/.gitignore b/.gitignore
index 7041d37..74d01cb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -56,7 +56,6 @@ 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
 tests/unit/test_utils_expand_path
 kernel_all_events_basic
 kernel_event_basic
diff --git a/src/common/utils.c b/src/common/utils.c
index d28e313..2f93cbe 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -164,73 +164,6 @@ error:
 }
 
 /*
- * 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;
-}
-
-/*
  * Make a full resolution of the given path even if it doesn't exist.
  * This function uses the utils_partial_realpath function to resolve
  * symlinks and relatives paths at the start of the string, and
diff --git a/src/common/utils.h b/src/common/utils.h
index 036e416..c23dfb9 100644
--- a/src/common/utils.h
+++ b/src/common/utils.h
@@ -26,7 +26,6 @@
 #define MEBI_LOG2 20
 #define GIBI_LOG2 30
 
-char *utils_resolve_relative(const char *path);
 char *utils_partial_realpath(const char *path, char *resolved_path,
 		size_t size);
 char *utils_expand_path(const char *path);
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index fa9c6a8..945dd00 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 test_utils_expand_path
+noinst_PROGRAMS += test_utils_parse_size_suffix test_utils_expand_path
 
 if HAVE_LIBLTTNG_UST_CTL
 noinst_PROGRAMS += test_ust_data
@@ -92,11 +92,6 @@ 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_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)
-
 # expand_path unit test
 test_utils_expand_path_SOURCES = test_utils_expand_path.c
 test_utils_expand_path_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
diff --git a/tests/unit/test_utils_resolve_relative.c b/tests/unit/test_utils_resolve_relative.c
deleted file mode 100644
index f43eeff..0000000
--- a/tests/unit/test_utils_resolve_relative.c
+++ /dev/null
@@ -1,98 +0,0 @@
-/*
- * Copyright (C) - 2013 Rapha?l Beamonte <raphael.beamonte at 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 35b31a4..561a94c 100644
--- a/tests/unit_tests
+++ b/tests/unit_tests
@@ -3,5 +3,4 @@ unit/test_session
 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] [lttng-tools PATCH 0/5] Follow up to the realpath(3) for unexistent paths

[David Goulet]

On 14 Nov (19:58:30), Rapha?l Beamonte wrote:
> David,
> 
> Following the discussions we had on IRC with Mathieu and the last
> mail exchange, here is a series of patch that should answer to our
> enquiries.
> 
> The first two patches are linked to the utils_expand_path unit test
> and correct its behavior and the expected results (some input was
> at first considered invalid, but during our discussions it was
> mentionned that it should be considered valid).
> 
> The next three ones introduce a new utils_partial_realpath that,
> as its name says it, allows to partially resolve a real path for
> a given path. This function is then used in utils_expand_path to
> resolve the paths, and the utils_resolve_relative function is
> removed as it is not necessary anymore in this new "setup".
> 
> Waiting for your comments,

Hey Raph,

I've created a symlink directory maze here to test theses patches and
everything seems to work fine but I got one wrong use case. I'll print my maze
here so you can recreate:

/tmp/test $
.
??? a
    ??? b
    ??? ??? c
    ??? ??? ??? g -> ../../../
    ??? ??? f -> ../e/
    ??? ??? h -> ../g/
    ??? ??? k -> c/g/
    ??? d -> b/c/
    ??? e
    ??? g -> d/

10 directories, 0 files

$ lttng create -o /tmp/test/a/g/../l/..
Session auto-20131115-103416 created.
Traces will be written in /tmp/test/a/b/l/..

Note the "/.." at the end. Relative or not, same output. Seems like "./" and
"../" *after* an unexisting path fails to parse. In this case, I would throw in
an error since there is simply no way of knowing that "l/" is going to point
to.

What I'm going to do here is to merge those 5 patches and wait for fix(es) that
we'll merge in the RC cycle. The feature is there just not perfect :). 2.4-rc1
is coming up this morning btw.

Thanks!
David

> Rapha?l
> 
> Rapha?l Beamonte (5):
>   Tests: add a check in test_utils_expand_path to avoid segfault if
>     result is NULL
>   Tests: move invalid tests in test_utils_expand_path that should be
>     valid
>   Introduce a new utils_partial_realpath function
>   Change the utils_expand_path function to use utils_partial_realpath
>   Remove the utils_resolve_relative function that is not useful anymore
> 
>  .gitignore                               |    1 -
>  src/common/utils.c                       |  256 ++++++++++++++++++------------
>  src/common/utils.h                       |    3 +-
>  tests/unit/Makefile.am                   |    7 +-
>  tests/unit/test_utils_expand_path.c      |    7 +-
>  tests/unit/test_utils_resolve_relative.c |   98 ------------
>  tests/unit_tests                         |    1 -
>  7 files changed, 158 insertions(+), 215 deletions(-)
>  delete mode 100644 tests/unit/test_utils_resolve_relative.c
> 
> -- 
> 1.7.10.4
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: Digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20131115/898d8720/attachment.pgp>

[lttng-dev] [lttng-tools PATCH 0/5] utils_expand_path fix

[Raphaël Beamonte]

Hello David!

Here is the fix needed to have the expected behavior for path that
ends with '/.' or '/..'. I also profit to send patches for the unit
test to allow to add tests using an existing tree and symlinks,
adding the missing copyright in utils.c and correcting the indentation
of one comment.

Waiting for your comments!

Thanks,
Rapha?l


Rapha?l Beamonte (5):
  Add missing copyright to utils.c
  Fix: comment indentation
  Fix: utils_expand_path now works for paths that ends with '/.' or
    '/..'
  Tests: add symlink tests for test_utils_expand_path
  Tests: add valid test cases to test_utils_expand_path

 src/common/utils.c                  |   30 +++++-
 tests/unit/test_utils_expand_path.c |  175 ++++++++++++++++++++++++++++++++++-
 2 files changed, 199 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 1/5] Add missing copyright to utils.c

[Raphaël Beamonte]

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 src/common/utils.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/src/common/utils.c b/src/common/utils.c
index 2f93cbe..3a4f850 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2012 - David Goulet <dgoulet at efficios.com>
+ * Copyright (C) 2013 - Rapha?l Beamonte <raphael.beamonte at gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License, version 2 only, as
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 2/5] Fix: comment indentation

[Raphaël Beamonte]

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 src/common/utils.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/common/utils.c b/src/common/utils.c
index 3a4f850..330c04e 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -198,7 +198,7 @@ char *utils_expand_path(const char *path)
 	if (*path != '/' && strncmp(path, "./", 2) != 0 &&
 			strncmp(path, "../", 3) != 0) {
 		snprintf(absolute_path, PATH_MAX, "./%s", path);
-		/* Else, we just copy the path */
+	/* Else, we just copy the path */
 	} else {
 		strncpy(absolute_path, path, PATH_MAX);
 	}
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 3/5] Fix: utils_expand_path now works for paths that ends with '/.' or '/..'

[Raphaël Beamonte]

Cases where the path given ended with '/.' or '/..' were not handled
properly using the utils_expand_path function and the resulting path
was still showing this end part. This fix aims to treat that last
part and 'expand' it as expected.

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 src/common/utils.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/common/utils.c b/src/common/utils.c
index 330c04e..ce25f23 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -178,6 +178,8 @@ LTTNG_HIDDEN
 char *utils_expand_path(const char *path)
 {
 	char *next, *previous, *slash, *start_path, *absolute_path = NULL;
+	char *last_token;
+	int is_dot, is_dotdot;
 
 	/* Safety net */
 	if (path == NULL) {
@@ -241,6 +243,31 @@ char *utils_expand_path(const char *path)
 				absolute_path, PATH_MAX);
 	}
 
+	/* Identify the last token */
+	last_token = strrchr(absolute_path, '/');
+
+	/* Verify that this token is not a relative path */
+	is_dotdot = (strcmp(last_token, "/..") == 0);
+	is_dot = (strcmp(last_token, "/.") == 0);
+
+	/* If it is, take action */
+	if (is_dot || is_dotdot) {
+		/* For both, remove this token */
+		*last_token = '\0';
+
+		/* If it was a reference to parent directory, go back one more time */
+		if (is_dotdot) {
+			last_token = strrchr(absolute_path, '/');
+
+			/* If there was only one level left, we keep the first '/' */
+			if (last_token == absolute_path) {
+				last_token++;
+			}
+
+			*last_token = '\0';
+		}
+	}
+
 	return absolute_path;
 
 error:
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 4/5] Tests: add symlink tests for test_utils_expand_path

[Raphaël Beamonte]

These new test cases allows to test the utils_expand_path
function with an existing tree using symlinks. It allows
to verify the right behavior of the function in complex
situation.

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 tests/unit/test_utils_expand_path.c |  173 ++++++++++++++++++++++++++++++++++-
 1 file changed, 168 insertions(+), 5 deletions(-)

diff --git a/tests/unit/test_utils_expand_path.c b/tests/unit/test_utils_expand_path.c
index bb460a2..6b809b8 100644
--- a/tests/unit/test_utils_expand_path.c
+++ b/tests/unit/test_utils_expand_path.c
@@ -21,6 +21,9 @@
 #include <stdlib.h>
 #include <limits.h>
 
+#include <sys/stat.h>
+#include <sys/types.h>
+
 #include <tap/tap.h>
 
 #include <src/common/utils.h>
@@ -35,6 +38,16 @@ struct valid_test_input {
 	char *absolute_part;
 };
 
+struct tree_symlink {
+	char *orig;
+	char *dest;
+};
+
+struct symlink_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"	},
@@ -62,6 +75,39 @@ char **valid_tests_expected_results;
 static const int num_valid_tests =
 		sizeof(valid_tests_inputs) / sizeof(valid_tests_inputs[0]);
 
+/* Symlinks test cases */
+char tree_origin[] = "/tmp/test_utils_expand_path.XXXXXX";
+
+static const char * const tree_dirs[] = {
+	"a",
+	"a/b",
+	"a/b/c",
+	"a/e",
+};
+static const int num_tree_dirs =
+		sizeof(tree_dirs) / sizeof(tree_dirs[0]);
+
+static struct tree_symlink tree_symlinks[] = {
+	{ "a/d",			"b/c/"		},
+	{ "a/g",			"d/"		},
+	{ "a/b/f",			"../e/"		},
+	{ "a/b/h",			"../g/"		},
+	{ "a/b/k",			"c/g/"		},
+	{ "a/b/c/g",			"../../../"	},
+};
+static const int num_tree_symlinks =
+		sizeof(tree_symlinks) / sizeof(tree_symlinks[0]);
+
+static struct symlink_test_input symlink_tests_inputs[] = {
+	{ "a/g/../l/.",			"a/b/l"		},
+	{ "a/g/../l/./",		"a/b/l/"	},
+	{ "a/g/../l/..",		"a/b"		},
+	{ "a/g/../l/../",		"a/b/"		},
+	{ "a/b/h/g/",			""		},
+};
+static const int num_symlink_tests =
+		sizeof(symlink_tests_inputs) / sizeof(symlink_tests_inputs[0]);
+
 /* Invalid test cases */
 static char *invalid_tests_inputs[] = {
 	NULL,
@@ -69,6 +115,13 @@ static char *invalid_tests_inputs[] = {
 static const int num_invalid_tests =
 		sizeof(invalid_tests_inputs) / sizeof(invalid_tests_inputs[0]);
 
+#define ERRSIZE 100
+char errmsg[ERRSIZE];
+static void printerr(char *msg)
+{
+	fprintf(stderr, "test_utils_expand_path: error: %s\n", msg);
+}
+
 int prepare_valid_results()
 {
 	int i;
@@ -85,7 +138,7 @@ int prepare_valid_results()
 	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");
+			printerr("malloc expected results");
 			return 1;
 		}
 
@@ -124,14 +177,96 @@ int free_valid_results()
 	return 0;
 }
 
+int prepare_symlink_tree()
+{
+	int i;
+	char tmppath[PATH_MAX];
+
+	/* Create the temporary directory */
+	if (mkdtemp(tree_origin) == NULL) {
+		printerr("mkdtemp");
+		goto error;
+	}
+
+	/* Create the directories of the test tree */
+	for (i = 0; i < num_tree_dirs; i++) {
+		snprintf(tmppath, PATH_MAX, "%s/%s", tree_origin, tree_dirs[i]);
+
+		if (mkdir(tmppath, 0755) != 0) {
+			snprintf(errmsg, ERRSIZE, "mkdir %s", tmppath);
+			printerr(errmsg);
+			goto error;
+		}
+	}
+
+	/* Create the symlinks of the test tree */
+	for (i = 0; i < num_tree_symlinks; i++) {
+		snprintf(tmppath, PATH_MAX, "%s/%s",
+				tree_origin, tree_symlinks[i].orig);
+
+		if (symlink(tree_symlinks[i].dest, tmppath) != 0) {
+			snprintf(errmsg, ERRSIZE, "symlink %s to %s",
+					tmppath, tree_symlinks[i].dest);
+			printerr(errmsg);
+			goto error;
+		}
+	}
+
+	return 0;
+
+error:
+	return 1;
+}
+
+int free_symlink_tree()
+{
+	int i;
+	char tmppath[PATH_MAX];
+
+	/* Remove the symlinks from the test tree */
+	for (i =  num_tree_symlinks - 1; i > -1; i--) {
+		snprintf(tmppath, PATH_MAX, "%s/%s",
+				tree_origin, tree_symlinks[i].orig);
+
+		if (unlink(tmppath) != 0) {
+			snprintf(errmsg, ERRSIZE, "unlink %s", tmppath);
+			printerr(errmsg);
+			goto error;
+		}
+	}
+
+	/* Remove the directories from the test tree */
+	for (i = num_tree_dirs - 1; i > -1; i--) {
+		snprintf(tmppath, PATH_MAX, "%s/%s", tree_origin, tree_dirs[i]);
+
+		if (rmdir(tmppath) != 0) {
+			snprintf(errmsg, ERRSIZE, "rmdir %s", tmppath);
+			printerr(errmsg);
+			goto error;
+		}
+	}
+
+	/* Remove the temporary directory */
+	if (rmdir(tree_origin) != 0) {
+		snprintf(errmsg, ERRSIZE, "rmdir %s", tree_origin);
+		printerr(errmsg);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	return 1;
+}
+
 static void test_utils_expand_path(void)
 {
 	char *result;
+	char name[100], tmppath[PATH_MAX];
 	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);
@@ -141,9 +276,23 @@ static void test_utils_expand_path(void)
 		free(result);
 	}
 
+	/* Test symlink tree cases */
+	int treelen = strlen(tree_origin) + 1;
+	for (i = 0; i < num_symlink_tests; i++) {
+		sprintf(name, "symlink tree test case: [tmppath/]%s",
+				symlink_tests_inputs[i].input);
+
+		snprintf(tmppath, PATH_MAX, "%s/%s",
+				tree_origin, symlink_tests_inputs[i].input);
+		result = utils_expand_path(tmppath);
+		ok(result != NULL && strcmp(result + treelen,
+					symlink_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_expand_path(invalid_tests_inputs[i]);
@@ -156,16 +305,30 @@ static void test_utils_expand_path(void)
 
 int main(int argc, char **argv)
 {
+	if (prepare_symlink_tree() != 0) {
+		goto error_mkdir;
+	}
+
 	if (prepare_valid_results() != 0) {
-		return 1;
+		goto error_malloc;
 	}
 
-	plan_tests(num_valid_tests + num_invalid_tests);
+	plan_tests(num_valid_tests + num_invalid_tests + num_symlink_tests);
 
 	diag("utils_expand_path tests");
 
 	test_utils_expand_path();
 
 	free_valid_results();
+	free_symlink_tree();
+
 	return exit_status();
+
+error_malloc:
+	free_valid_results();
+
+error_mkdir:
+	free_symlink_tree();
+
+	return 1;
 }
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 5/5] Tests: add valid test cases to test_utils_expand_path

[Raphaël Beamonte]

Signed-off-by: Rapha?l Beamonte <raphael.beamonte at gmail.com>
---
 tests/unit/test_utils_expand_path.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/unit/test_utils_expand_path.c b/tests/unit/test_utils_expand_path.c
index 6b809b8..65582ff 100644
--- a/tests/unit/test_utils_expand_path.c
+++ b/tests/unit/test_utils_expand_path.c
@@ -70,6 +70,8 @@ static struct valid_test_input valid_tests_inputs[] = {
 	{ ".",				".",		""		},
 	{ "/../a/b/c/d/e",		"",		"/a/b/c/d/e"	},
 	{ "/a/b/c/d/../../../../../e",	"",		"/e"		},
+	{ "/..",			"",		"/"		},
+	{ "/a/..",			"",		"/"		},
 };
 char **valid_tests_expected_results;
 static const int num_valid_tests =
-- 
1.7.10.4

[lttng-dev] [lttng-tools PATCH 0/5] utils_expand_path fix

[David Goulet]

Fixes every use case I had that failed previously :).

Merged. Thanks!

David

On 23 Nov (17:32:23), Rapha?l Beamonte wrote:
> Hello David!
> 
> Here is the fix needed to have the expected behavior for path that
> ends with '/.' or '/..'. I also profit to send patches for the unit
> test to allow to add tests using an existing tree and symlinks,
> adding the missing copyright in utils.c and correcting the indentation
> of one comment.
> 
> Waiting for your comments!
> 
> Thanks,
> Rapha?l
> 
> 
> Rapha?l Beamonte (5):
>   Add missing copyright to utils.c
>   Fix: comment indentation
>   Fix: utils_expand_path now works for paths that ends with '/.' or
>     '/..'
>   Tests: add symlink tests for test_utils_expand_path
>   Tests: add valid test cases to test_utils_expand_path
> 
>  src/common/utils.c                  |   30 +++++-
>  tests/unit/test_utils_expand_path.c |  175 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 199 insertions(+), 6 deletions(-)
> 
> -- 
> 1.7.10.4
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: Digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20131125/f9e88635/attachment-0001.pgp>

[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 at 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 at 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] [lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories

[David Goulet]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: Digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20131113/16bee95d/attachment.pgp>

[lttng-dev] [lttng-tools PATCH 2/2] 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 at gmail.com>
---
 src/common/utils.c |   72 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 21 deletions(-)

diff --git a/src/common/utils.c b/src/common/utils.c
index 7b52760..b68f12e 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -92,7 +92,7 @@ int utils_resolve_relative(char *path)
 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 */
@@ -100,35 +100,65 @@ 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;
+		/* If the path is just a directory name */
+		if (strchr(path, '/') == NULL || strchr(path, '/') == last_char) {
+			cut_path = strdup(".");
+		/* Else, we have to analyse the full path */
+		} else {
+			/* 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;
+				}
+			}
+
+			/* Cut last token from original path */
+			cut_path = strndup(path, end_path - path);
 		}
-		goto error;
+
+		/* 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 */
+		strncat(expanded_path, "/", PATH_MAX - strlen(expanded_path) - 1);
+		strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
 	}
 
-	/* Add end part to expanded path */
-	strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
+	/* Resolve the internal './' and '../' strings */
+	if (utils_resolve_relative(expanded_path) != 0) {
+		goto error;
+	}
 
 	free(cut_path);
 	return expanded_path;
-- 
1.7.10.4