[SSSD] [PATCHES] New functionality for refarray and new comment object

Stephen Gallagher sgallagh at redhat.com
Tue Apr 27 17:04:18 UTC 2010


On 04/27/2010 12:32 PM, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>> On 04/26/2010 04:38 PM, Dmitri Pal wrote:
>>
>>> Hello,
>>>
>>> Patch 1: New functionality for refarray. Some basic functionality was
>>> missing. Now it is added.
>>>
>>
>> Nitpick: in ref_array_replace(), your trace message lists
>> ref_array_insert().  It might not be a bad idea in a separate patch to
>> change TRACE_FLOW_* to use __FUNCTION__ instead of passing it the
>> function name explicitly. It's less error-prone.
>>
>
> Good idea. Did not occur to me earlier.
> But It would require going through every function and changing the code
> to use the macro.
> I suggest we reserve this for the new interfaces and object we add.
> So here I will just fix the typo. Ok?
>

As I said, updating that macro should be a future patch. Just fix the 
typo for now.

>
>> In ref_array_reset(), you need to document very clearly the order that
>> the callbacks will be invoked or it needs to be documented that the
>> order is not guaranteed. I think the latter is probably a better move,
>> since it grants us leeway in the future, as well encouraging the
>> consumer not to right order-dependent callbacks.
>>
>>
>>
> Same patch or separate patch?

Please amend this patch.

>
>
>
>>> Patch 2: New object to preserve and store comments in the INI file.
>>> Takes advantage of the refarray interface.
>>> This patch includes a separate unit test. Nothing uses this object yet.
>>> It will be a part of the new more complex value object I am currently
>>> working on.
>>>
>>
>> I'd prefer ini_comment_is_valid() rather than ini_comment_is_val().
>> 'val' usually means 'value', so this is confusing.
>>
>
> Ok.
>
>> Also, I'd have to check with RFC822 on this, but I think leading spaces
>> followed by ; or # should still be a valid comment.
>>
>
> So did you check it? If not I can do it myself.

Actually, I withdraw this. Leading spaces in RFC 822 are only allowed 
when denoting multi-line values

>
>> Please reduce indentation on the mode handling in ini_comment_modify().
>> It's much too far right. Our coding guideline specifies four spaces.
>>
>
> Sure I will take a look.
>
>
>> Otherwise, this looks good to me.
>>
>>
>>
>>
> Thanks for the review!
>


-- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/



More information about the sssd-devel mailing list