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

Fri Jul 31 19:04:08 UTC 2009

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

On 07/22/2009 06:10 PM, Dmitri Pal wrote:
> Patch 1 (small)
> 
> [PATCH] INI Simple fix to properly process multi value config parameters.
> Also fixed a typo in the header file.
> 

Ack

> Patch 2 (big)
> 
> [PATCH] ELAPI Next round of functionality - logging part of the interface
> 
> a) Added the main logging interface which
> allows creating dispatcher and logging messages or events.
> Can't actually log anything yet since the sinks are stubbed out.
> b) Made default template be a part of the default
> dispatcher.
> c) Updated UNIT test.
> d) Added preview of the async interface (subject to change
> when I actually try to implement it and see what is missing).
> e) Some of the calls are stubbed out but they are there
> to indicate where next round of work will be.
> 
> 

Nack.

Please squash the patch "ELAPI Adding Core Functionality" into this
second patch. Neither one makes sense without the other.

I don't like the --with-config-file and --with-config-dir configure
parameters. It's usually preferred to keep the filename constant (unless
overridden at runtime by a user) and just specify its location.

I'd suggest using the following:
- --with-config-dir [SYSCONFDIR/elapi]
- --with-extra-config-dir [SYSCONFDIR/elapi/apps.d]

You can't pass APP_NAME_SIZE as a string and then compare
strlen(appname) to it. You're comparing it to the pointer address of the
static string. You need to convert it to an integer either in the
configure script or in the C code before you can use it that way.

- --with-error-file doesn't make sense. There's no reason they should need
to change the name of the file. Possibly its location (if they want it
somewhere other then CWD)

Your ELAPI async interface is incomplete and belongs in a separate patch
in any case. I will review this in more detail separately.

elapi_event.c:
add_host_identity()
Please don't use memset on large static character arrays. It's a
wasteful operation (in this case, it's 1025 memory writes where one is
sufficient). It's much better to set hostname[0] = 0;  and then just
ensure null termination later if you copy data into it. Memsetting a
character array leads to the bad assumption that you are guaranteed a
termination. For example, when you use gethostname() later, if the
hostname is longer than NI_MAXHOST for some reason, you are not
guaranteed null termination. So you should manually insert a null
terminator at hostname[NI_MAXHOST]. You do the same thing in several
other places in this function, and throughout your code. getnameinfo()
guarantees null termination, so memset is completely wasted here.

getifaddrs() is not portable. It is implemented only for systems
corresponding to BSDi, and even on those systems, it varies widely. If
you intend to use this, it needs to be controlled by a configure flag.

Please prefer comparisons to EOK or EXIT_SUCCESS instead of 0.
Comparison to 0 can imply "false". Similarly,
if (!gethostname(hostname, NI_MAXHOST)) hnm = hostname;
doesn't read well.

Comparing against LOCALHOSTDOMAIN is unnecessary, because it will
already have matched LOCALHOST (since you are limiting the search to the
length of LOCALHOST).  I'd rather see you actually perform a strcasecmp
rather than a strncasecmp here, because it's theoretically possible that
someone could choose localhost1.example.com for some ridiculous reason,
and we'd be throwing it away. It's safe to omit the search limit because
strcasecmp() will stop at the end of the shorter string.

Why are you doing a string compare of the printable versions of the
address vs. LOCALADDRESS[V6]? It would be much faster to do a numeric
compare of sockaddr_in->in_addr->s_addr;

Similarly, why are we storing the host address as a string instead of as
an integer? Seems like a waste of space.

add_base_elements():
pass_base = base; is an implicit signed/unsigned conversion on some
platforms. It would be best if you defined them both as uint32_t.

interprete_key():
For the sake of the sanity of anyone using the code, please spell
"interpret" correctly. There's no need to return the length of the
strings in the function, since you know they're null-terminated.
Returning the length is only useful in arrays without null-termination.

convert_bool():
I'd prefer that you use strcasecmp rather than strncasecmp unless you
want to allow "Truest" and "Falsely" to match TRUE and FALSE as well.

process_arg_list():
strndup() is a GNU extension, it is not portable. Since you control
interpret_key(), you know that property will never be unreasonably large
and will always be null-terminated, so it's safe to use strdup() here.
Use the aforementioned ELAPI_KEY_TYPE_* defines here instead of just
if/else.

Prefer "elapi_create_event_va" to "with_args". Both elapi_create_event()
and elapi_create_event_with_args() take arguments.

elapi_internal.c:
This is a matter of personal preference, but I prefer
_elapi_sink_handler() over elapi_int_sink_handler() to denote an
internal function. I am aware that we have an _int suffix in some places
in the code, but I'd like to recommend that we move to the underscore
prefix. I will send out a coding style change request separately.

elapi_log.c:
No issues beyond those mentioned above.

> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkpzQCMACgkQeiVVYja6o6NEbgCfaepXzrEgxxk+TNackeJRf5Qv
V3kAnjuxlTBVY3EAUFp6wB7GkH9JJBfC
=G19K
-----END PGP SIGNATURE-----