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

Dmitri Pal dpal at redhat.com
Wed Apr 28 17:08:32 UTC 2010


Stephen Gallagher wrote:
> 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!
>>
>>     
>
>
>   
Updated patches attached.

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001--REFARRAY-Extending-refarray-interface.patch
Type: text/x-patch
Size: 17251 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100428/7ee4a8dd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002--INI-Introducing-a-comment-object.patch
Type: text/x-patch
Size: 24941 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100428/7ee4a8dd/attachment-0001.bin>


More information about the sssd-devel mailing list