[DTrace-devel] [PATCH] Add support for cleanpath() subroutine

[Eugene Loh]

Thanks for the feedback.  Anyhow...

On 11/23/23 07:51, Alan Maguire wrote:
> On 21/11/2023 04:56, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Replace all "//" with "/".
>> Replace all "/./" with "/".
>> Replace all "../foo/" with "/".
>>
> A few questions about this below...
>
>> Notice that the results differ in some cases from what was
>> done in legacy DTrace:
>>
>>      input string         current patch     legacy DTrace
>>
>>      ""                   "."               ""
>>
> I wonder about this substitution. If an empty path in a kernel data
> structure connotes "no file is associated with this object", replacing
> it with "." seems like it could be misleading.

Good point.  Also, would probably simplify the code.  Hopefully, others 
will chime in, but I'm happy to make this change.

>>      "/foo/bar/baz/"      "/foo/bar/baz"    "/foo/bar/baz/"
> Question about this one below...
>
>>      "/."                 "/"               "/."
>>      "a/."                "a"               "a/."
>>      "./a"                "a"               "./a"
>>
>>      "../.."              "../.."           "."
>>      "a/.."               "."               "a"
>>      "a/../.."            ".."              "a"
>>
>>      "a/../b"             "b"               "/b"
>>
> ...and presumably "/a/../b" becomes "/b", right?	

Yes.  I think I didn't mention this because both legacy and this patch 
agree on "/b".

>> In the first case, one can argue about which implementation is
>> "correct."
>>
>> In the next cases, there are unnecessary trailing '/' or '.' or
>> both or unnecessary leading "./".  They would seem to be wrong.
>>
>> In the next cases, the legacy results are clearly wrong, not
>> handling ".." correctly.
>>
>> In the final case, a relative path is converted into an absolute
>> path, which is also clearly incorrect.
>>
> This all seems fairly consistent with the description at

Since I describe differences between the patch and legacy, let me make 
sure.  What is consistent with the Solaris docs?  The patch's behavior, 
I hope?  (The legacy behavior strikes me as egregiously wrong in some 
cases.)

> https://docs.oracle.com/en/operating-systems/solaris/oracle-solaris/11.4/dtrace-guide/cleanpath-subroutine.html
>
> ...aside from (perhaps)
>
>       "/foo/bar/baz/"      "/foo/bar/baz"    "/foo/bar/baz/"
>
> I think the question is if the trailing "/" is a valid redundant
> element to eliminate, right? Whether it is redundant probably
> hinges on how the cleanpath()ed path will be used; it could be argued
> that it's potentially useful information that it is a directory rather
> than a file. If for example we were using cleanpath()ed paths as an
> aggregation key, seeing which were files versus directories could
> potentially be useful data. Looking around at scripts that use
> cleanpath(), most use it to output the cleaned-up path, so maybe it's
> not a huge deal, but it does seem to stretch the concept of redundancy
> somewhat beyond the original intent. That's just my interpretation
> though, I don't feel too strongly about it.

Thanks.  Yeah, I don't know.  I used to output a trailing '/' if and 
only if the input had one, but finally decided to drop that complexity 
given no concrete reason to keep it.  You articulate such a reason 
well.  I'm inclined to restore that behavior, especially if someone 
chimes in in support.