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

[Stephen Gallagher]

On 07/14/2009 06:48 PM, John Dennis wrote:
> 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.
> 

Sorry, I found a couple remaining minor issues.

You were creating a path_utils.pc file directly, instead of a
path_utils.pc.in that gets its install path data from configure.

The minor typo I mentioned in my previous review was unchanged.

I am attaching a patch to this email that corrects these issues (as they
are so minor that I figured I'd save you the trouble). Please review
them yourself, and if you approve of them as-is, I will squash it into
your patch and push it.

-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Fix-path_utils.pc.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090715/9676c518/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090715/9676c518/attachment.sig>