Mirror of the lttng-dev mailing list
 help / color / mirror / Atom feed
From: raphael.beamonte@gmail.com (Raphaël Beamonte)
Subject: [lttng-dev] [lttng-tools PATCH 1/4] Introduce a new utils_resolve_relative function
Date: Thu, 14 Nov 2013 12:07:31 -0500	[thread overview]
Message-ID: <CAE_Gge1=gN2K1yVQ=1gXqH3Z-NYOKWrrW807xNRjb2P6XbmppA@mail.gmail.com> (raw)
In-Reply-To: <254665384.67131.1384438761175.JavaMail.zimbra@efficios.com>

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



  reply	other threads:[~2013-11-14 17:07 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-08  4:32 [lttng-dev] [lttng-tools PATCH 0/2] utils_expand_path as realpath(3) for non-existing directories Raphaël Beamonte
2013-11-08  4:32 ` [lttng-dev] [lttng-tools PATCH 1/2] Introduce a new utils_resolve_relative function Raphaël Beamonte
2013-11-12 19:51   ` David Goulet
2013-11-13  5:34     ` [lttng-dev] [lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories Raphaël Beamonte
2013-11-13  5:34       ` [lttng-dev] [lttng-tools PATCH 1/4] Introduce a new utils_resolve_relative function Raphaël Beamonte
2013-11-14 14:19         ` Mathieu Desnoyers
2013-11-14 17:07           ` Raphaël Beamonte [this message]
2013-11-13  5:34       ` [lttng-dev] [lttng-tools PATCH 2/4] Tests: Add test_utils_resolve_relative to unit tests Raphaël Beamonte
2013-11-13  5:34       ` [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function Raphaël Beamonte
2013-11-14  2:44         ` Mathieu Desnoyers
2013-11-14  3:27           ` Raphaël Beamonte
2013-11-14 13:46             ` Mathieu Desnoyers
2013-11-14 14:10         ` Mathieu Desnoyers
2013-11-14 16:00           ` Raphaël Beamonte
2013-11-14 16:14             ` Mathieu Desnoyers
2013-11-15  0:58               ` [lttng-dev] [lttng-tools PATCH 0/5] Follow up to the realpath(3) for unexistent paths Raphaël Beamonte
2013-11-15  0:58                 ` [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
2013-11-15  0:58                 ` [lttng-dev] [lttng-tools PATCH 2/5] Tests: move invalid tests in test_utils_expand_path that should be valid Raphaël Beamonte
2013-11-15  0:58                 ` [lttng-dev] [lttng-tools PATCH 3/5] Introduce a new utils_partial_realpath function Raphaël Beamonte
2013-11-15  0:58                 ` [lttng-dev] [lttng-tools PATCH 4/5] Change the utils_expand_path function to use utils_partial_realpath Raphaël Beamonte
2013-11-15  0:58                 ` [lttng-dev] [lttng-tools PATCH 5/5] Remove the utils_resolve_relative function that is not useful anymore Raphaël Beamonte
2013-11-15 15:45                 ` [lttng-dev] [lttng-tools PATCH 0/5] Follow up to the realpath(3) for unexistent paths David Goulet
2013-11-23 22:32                   ` [lttng-dev] [lttng-tools PATCH 0/5] utils_expand_path fix Raphaël Beamonte
2013-11-23 22:32                     ` [lttng-dev] [lttng-tools PATCH 1/5] Add missing copyright to utils.c Raphaël Beamonte
2013-11-23 22:32                     ` [lttng-dev] [lttng-tools PATCH 2/5] Fix: comment indentation Raphaël Beamonte
2013-11-23 22:32                     ` [lttng-dev] [lttng-tools PATCH 3/5] Fix: utils_expand_path now works for paths that ends with '/.' or '/..' Raphaël Beamonte
2013-11-23 22:32                     ` [lttng-dev] [lttng-tools PATCH 4/5] Tests: add symlink tests for test_utils_expand_path Raphaël Beamonte
2013-11-23 22:32                     ` [lttng-dev] [lttng-tools PATCH 5/5] Tests: add valid test cases to test_utils_expand_path Raphaël Beamonte
2013-11-25 15:51                     ` [lttng-dev] [lttng-tools PATCH 0/5] utils_expand_path fix David Goulet
2013-11-13  5:34       ` [lttng-dev] [lttng-tools PATCH 4/4] Tests: Add test_utils_expand_path to unit tests Raphaël Beamonte
2013-11-13 21:28       ` [lttng-dev] [lttng-tools PATCH 0/4] Update to utils_expand_path as realpath(3) for non-existing directories David Goulet
2013-11-08  4:32 ` [lttng-dev] [lttng-tools PATCH 2/2] Correct the behavior of the utils_expand_path function Raphaël Beamonte

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAE_Gge1=gN2K1yVQ=1gXqH3Z-NYOKWrrW807xNRjb2P6XbmppA@mail.gmail.com' \
    --to=raphael.beamonte@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox