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

Dmitri Pal dpal at redhat.com
Tue Aug 11 14:00:15 UTC 2009


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.
>
> 2) conf_macros.m4: line 25, seems like a cut-n-paste error, I think it
> should set elapiconfappdir=
>
> 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.
>
> 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?
>
> 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?
>
> 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
>
> 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
>
> 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.
>
> 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?
>
> 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
>
>     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

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


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




More information about the sssd-devel mailing list