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(a)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/