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

Stephen Gallagher sgallagh at redhat.com
Wed Apr 28 17:19:50 UTC 2010


Ack.

On 04/28/2010 01:08 PM, Dmitri Pal wrote:
> 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.
>
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel


-- 
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