[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 at 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