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

Jakub Hrozek jhrozek at redhat.com
Tue Aug 11 13:04:49 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqBbHEACgkQHsardTLnvCW+JACfaMv5Gaz88Go5z7yhBZuG9Vfs
dSsAoJ7bEw/SJntw0owWsI986gFAMjVt
=HIGU
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list