[Freeipa-devel] [PATCH] add path_utils to common

[John Dennis]

On 07/14/2009 03:25 PM, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/14/2009 02:02 PM, John Dennis wrote:
>> This is the first of several patches to add code to repository for the
>> audit log watching code. I needed a variety of file system path
>> manipulation functions which either weren't in standard libraries or
>> didn't work as needed. Since this is general utility code it was broken
>> out from the rest of the log watch code to permit reuse. The code has
>> been heavily beat upon and sails through valgrind without any errors.
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> Nack.

New patch attached which addresses the issues Stephen raised. Thank you 
Stephen for your careful reading.

>
> path_utils_error_string():
> PATH_UTILS_ERROR_NOT_FULLY_NORALAIZED should probably be
> PATH_UTILS_ERROR_NOT_FULLY_NORMALIZED. Also, there should be
> capitalization of "path" in the string there.
>
> Furthermore, should path_utils_error_string() be localized? I'm not
> saying in this patch, but it looks like a good candidate.

easy, done.

> get_basename():
> It would be wise to put in a check that base_name_size<  PATH_MAX or you
> could get truncation. In that case, I'd think you might want to have
> this function return either NULL or better yet an error code.

I put in a slightly different check which I think is more robust. 
Truncation can also occur if the size of the passed in buffer is too 
small. The check is to see if strncpy exhausted the available buffer 
space, which will be true if the last byte in the buffer is not zero. 
This check has been added in numerous places.

> get_dirname():
> You need to check for ERANGE after getcwd(). It's possible that you're
> getting a path back from a foreign filesystem that supports longer paths
> than PATH_MAX.

done

> get_directory_and_base_name():
> Same concerns as get_dirname() and get_basename(). Also, is this
> function useful? (I realize it's one fewer copy to tmp_path than calling
> get_basename and get_dirname, but it seems like a bit of code waste)

The reason for this extra function is because I discovered it was very 
common to want both the basename and the directory, so the frequency of 
use justified the optimization.

> path_concat():
> if (dst>= dst_end) should be if (dst>  dst_end). You've already
> accounted for the null terminator when defining dst_end.
>
> make_path_absolute():
> Same concerns as path_concat().

good catch, done.

> Finally, you have a ton of tabs and trailing spaces hanging around.

silly me, I thought I had taken care of that but I guess not, it's done now.

-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-path_utils-filesystem-path-manipulation-utility.patch
Type: text/x-patch
Size: 33690 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090714/fe96316d/attachment.bin>