[SSSD] [Freeipa-devel] [PATCHES] one for INI another for ELAPI

Dmitri Pal dpal at redhat.com
Tue Aug 11 19:29:35 UTC 2009


Dmitri Pal wrote:
> Jakub Hrozek wrote:
>   
>> On 08/10/2009 08:27 PM, Stephen Gallagher wrote:
>>     
>>> Ack. Waiting for a reply from Jakub as well before pushing these
>>>       
>> patches.
>>
>> Patch 0001 (INI fix): ACK
>>
>> Patch 0002 (ELAPI): I have some comments and questions. Most of them are
>> just style comments, but I think at least 2) and 8) should be fixed
>> before we push the patch.
>>
>> The code builds cleanly, passes all its unit tests. I have not tested
>> functionality any further.
>>
>> 1) what is ERR_FILE=@elapierrfile@ in Makefile.am? It appears to be some
>> kind of leftover, it is not set by configure or used afterwards.
>>
>>     
It was. Removed.

>> 2) conf_macros.m4: line 25, seems like a cut-n-paste error, I think it
>> should set elapiconfappdir=
>>
>>     
You are right. Corrected.

>> 3) conf_macros.m4: the default values are usually given in form of
>> [value], the file sometime uses [key=value], sometime [default=value]
>> and sometimes [value]. It would be nice to stick to one standard.
>>
>>     
Cleaned to be [value].

>> 4) In general (this is maybe question also for the others not a review
>> item), is there a reason for passing the macros containing
>> configure-time variables as -DMACRO instead of using AC_DEFINE_UNQUOTED
>> and pulling them out of config.h? Is is just a matter of style?
>>
>>     
Following Simo's comment did not do anything with it.

>> 5) I see that get_default_template was moved and is now non-static. But
>> at the same time, it is not declared in any header. Is it missing from
>> the headers or should it be static as previously?
>>
>>     
Exposed it in the header elapi_event.h.

>> 6) similar issue, just not strictly related to this patch, but in
>> elapi_event.c, the "undefined", "str_yes" etc. are not declared as
>> static but not used outside elapi_event.c
>>
>>     
Those will probably be translatable strings so it will be handled later
when I add language support.
I opened a ticket not to forget.
https://fedorahosted.org/sssd/ticket/98

>> 7) elapi_log.c - does it make sense for variables such as
>> default_config_file to be declared "static const char *"? Or perhaps
>> even #defined? Also the variable elapi_close_registered could be made
>> static
>>
>>     
Added "static" to those.

>> 8) in elapi_log.c, the check for handle->appname == NULL on line 228
>> should probably be moved before the TRACE_FLOW_STRING on line 225 as the
>> trace macro tries to access the memory in handle->appname. Also maybe
>> it's OK to just return ENOMEM and don't bother with errno.
>>
>>     
As for errno it is a valid way of handling error so I guess no change
needed.
I prefer errno, I can't explain why...

Added ticket to address it in a separate patch since fix should be in
the macro not in the ELAPI code.
https://fedorahosted.org/sssd/ticket/99

>> 9) why the (void) in elapi_log.c:332? Is it to make clear that we don't
>> care about the return value from col_traverse_collection?
>>
>>     
This is a cleanup, we are destroying object. It is a best effort.
Returning error there is like allowing free() to return an error.
What would you do with such an error?
This is why it is ignored.

>> 10) elapi_internal.c:elapi_internal_dump_errors_to_file - maybe the
>> strftime should use E_TIMESTAMP_FORMAT instead of hardcoded "%F" to be
>> consistent
>>
>>     
Done.

>>     Jakub
>>
>>
>>
>>     
> Thank you for the review. I will update and resubmit.
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>   

Thanks for review!
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-INI-Simple-fix-to-properly-process-multi-value-confi.patch
Type: text/x-patch
Size: 1708 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090811/bc8131c3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ELAPI-Next-round-of-functionality-logging-part-of.patch
Type: text/x-patch
Size: 80651 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090811/bc8131c3/attachment-0001.bin>


More information about the sssd-devel mailing list