[Freeipa-devel] [PATCH] initial commit of log watcher (lwatch)

[Stephen Gallagher]

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

On 07/23/2009 09:01 PM, Dmitri Pal wrote:
> 
> 9) SSSD and Common and Audit all three use tree different ways of
> tracing/debugging things.
> Shouldn't we get to some consistency there?
> It would be better if the log_msg be a macro from the very beginning.
> This way it would be easier
> to make it conditionally compiled debugging. Currently is is function
> and would require significant
> effort to go through the code and make it consistent. I would argue that
> a) Tracing/debugging should be conditionally compiled day 1
> b) Debugging strings do not require translation
> c) Tracing pattern should be consistent - every error should be traced
> otherwise it is hard to find problems.
> 

Debug messages should NEVER be conditionally compiled. For an
executable, they should always be set by a runtime flag, and for a
library they should always be set by an argument to the initialization
function or by an environment variable. It should always be possible for
someone running the code in a production environment to get additional
information about a crash without requiring an instrumented build.

Furthermore, "tracing" should be a special case of debugging (i.e. it
should just be a very high debug_level.

> Just something to think about...
> 
> 10)  Such code will not produce right results since there are multiple
> ways why function can return NULL and not in all cases
> the errno is set.
>        
>         if ((pw = new_path_watch(existing_directory_ancestor)) == NULL) {
> +            error = errno;
> +            log_msg(LOG_ERROR, _("%s: could not allocate new watch for
> \"%s\" (%s)\n"),
> +                    __FUNCTION__, existing_directory_ancestor,
> error_string(error));
> +            return error;
> 
> Better approach to such cases would be either
> 
> pw = new_path_watch(existing_directory_ancestor, &error);
> if (!pw) {
> ...
> 
> or which I prefer more:
> 
> error = new_path_watch(&pw, existing_directory_ancestor);
> if(error) {
> ...

This second approach is more in line with the rest of the code in the
SSSD. We do this all over the code. Also, our preference is for this
error code to remain in line with the errno set, so things like ENOENT
and ENOMEM are standard. (We also defined EOK which is identical to
EXIT_SUCCESS for convenience)

> 
> 11) From our design discussions I remember that  you mentioned that
> there was an issue with recursive directories and monitoring nested
> paths or something like this.
> It seems that find_existing_directory_ancestor() has to do with this issue.
> I was expecting to see some extended comment about this somewhere around
> create_watch() function.
> 
> 12) In search for some functions I looked at path_utils.
> First in general I am a bit scared about using static buffers to hold paths.
> Is it still an Ok practice? In the SSSD all code is allocated in
> collections
> and ini the only place  where the static buffer was used was some
> parsing of the
> file where the reasonable length of the line in the ini file is less
> than 100 bytes
> while buffer is about 64K. Should we use dynamic memory for paths?
> Start with malloc(MAX_PATH) and then realloc if needed?
> 

The idea behind MAX_PATH is that it's defined on every system to be the
largest legitimate value of a path that the system can handle
internally. malloc(MAX_PATH) doesn't gain anything over path[MAX_PATH]
except a need to manage the memory. I generally prefer, however, only to
allocate memory as-needed. This will come in handy in the future if
anyone should want to include this daemon on a limited-memory device
(such as a Linux-based router). That said, for speed and ease of use,
you can't beat this static buffer. I just balk at always using a full
memory page for every path, when in most cases you're going to need <
100 bytes. Considering that the whole purpose of this tool is to easily
manage large numbers of paths, that 4kiB (on Linux; 8kiB or more on some
other systems) can really add up.

> 13) I was amazed how well the path_util interface is defined and
> documented .
> I will consider using it whenever I need to deal with paths.
> 
> This is all for now.
> A general comment is that without running the code and playing with it
> for some time
> it is hard to find any specific issues (if any) in logic. On the surface
> things
> seem well thought through and the functions are not big and modular but
> lack of comments makes it really hard to digest.
> 


- -- 
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/

iEYEARECAAYFAkppn0UACgkQeiVVYja6o6Ps8wCgkDcTyeAVLbUMYilhGGoJBDiL
Cr4AnAlRVq+GYqLHzp8x/XPLDTfJghAk
=SEqh
-----END PGP SIGNATURE-----