Hi Ginnie,
Looking good, have some comments though ;)
General
-------
- Is it expected that the create_*() handler functions will be used by
anyone other then the add_handler() method?
- The main reason that I ask is that it would seem that really the
API consumers would be bothered with is the *_HDLR macros, which
are passed into add_handler(), which in turn is used to call the
correct create_*() method - it seems that these create_*() methods
are really internal implementation and should probably be static
methods in install_logging.c.
If these are internal methods, then it would be fine to return
PyObject (see previous e-mail response) since it wouldn't be
exposed as part of the API.
install_logging.c
------------------
- In each method, where _initialize_py() is being called, on failure,
there is a subsequent call to _report_error() and PyErr_Clear() - in
all cases this appears to be almost always the same, as such would
it not be better to put this in the _initialize_py() function itself
rather than repeating the code?
Some do seem to differ slightly - some calling PyErr_Print() and a
slightly different message, but these seem to be inconsistent which
is another reason it might be better to out in _initialize_py()
since they still appear to be trying to do achieve the same result.
- Each function seems to have it's own "function" local
variable which is the name of the function being run at the time.
This shouldn't be needed - the compiler already has such a value
which is the macro __func__ if the compiler flag
'-features=extensions' is specified - unfortunately it's not, maybe
this is something to ask RE/Gatekeepers about since it would be more
reliable than defining a local variable all the time in code. If
you don't go this route, the variable should probably be defined as
static and const too to avoid creating it each time a function is
called.
- _report_error(), log_message() and report_progress()
- The variable 'buf' in a vasprintf() method is not checked for a
NULL value or failure of the vasprintf() - instead its being
passed later to a call of PyString_FromString() or
es_set_err_data_str(), while these might handle the NULL, it would
be better to report a failure of the vasprintf() when it happens
rather than things progressing until later, thus masking the
location of the failure.
- _report_error()
- The first parameter to _report_error() is an 'out' paramater, but
nothing that calls it seems to make any use of it, is there a need
to have it - even where it is passed it, in all cases it's the
global value log_err_info.
If it is needed, would it be better to have as the return value?
line 148, ei_instance is not checked for NULL so it could fail
with a SEGV.
line 149, seems to be a duplicate of the RHS of the previous line,
and should be removed.
- The pramaeters 'func_name' and 'message' should most likely be
'const char*'.
- set_logger_class
- The parameter 'logger_class' should probably be 'const char*'
- line 221, the return value of pDict isn't checked for NULL and is
used immeditately after at line 224 - it is possible for this
value to be NULL?
- add_handler
- You seem to be creating pTupArgs and decrementing it's ref-count
several times, is it not posible to re-use the tuple rather than
creating more than once?
- log_message
- The parameter 'message' should probably be 'const char*'
- line 854, you have an fprintf() call here, probably should be
removed.
- line 871 - 877, you're using the paramater 'level' here without
first checking if it's non-NULL. Also, maybe all the 'strcmp()'
should be 'strncmp()' calls, but maybe it's overkill.
- get_logger
- The parameter 'logger_name' should probably be 'const char*'.
- report_progress
- The parameter 'message' should probably be 'const char*'
install_logging.h
------------------
- The header guard (#ifndef _INSTALL_LOGGING_H) should probably be
just after the license before the other #includes.
- lines 79-81, the macros defined here should probably have the term
logging in the name - e.g. LOGGING_ERR_PY_INIT - to remove clashes
with other macros.
handlers.h
----------
- lines 30-35 the type and enum values should probably include
'logging' in the names - e.g. logging_handler_type_t, and
LOGGING_FILE_HDLR.
handlers.c
----------
- Similar comment as in install_logging.c above w.r.t. use of local
variable 'function'.
- Still wondering if these should be private methods as opposed to
public methods.
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
