Hi Darren -
Thanks for the review....see my comments below. I'm not sure why, but I
never received your original responses, so I've pasted it below.
I've put my responses between asterisks...hopefully it isn't too confusing.
Thanks,
ginnie
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.
************************
They are only used by add_handler. I'll modify as you've suggested.
*************************
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.
*****************
It's not completely arbitrary. Sanjay, Keith and I had a discussion and
decided for set_logger_class and get_logger to generate PyErr_Print
since we won't want to continue if we can't get the logger going.
PyErr_Clear is used for the other functions.
I think because there is some method to the apparent madness, it might
be better to leave those calls within the function.
******************
- 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.
*******************
ok. Thanks.
*******************
- _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.
**********************
ok. I can add that check
**********************
- _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.
**********************
yes...I'll add a check for that.
***********************
line 149, seems to be a duplicate of the RHS of the previous line,
and should be removed.
**********************
yes. I'll remove that.
**********************
- The pramaeters 'func_name' and 'message' should most likely be
'const char*'.
*************
I'll change it.
*************
- set_logger_class
- The parameter 'logger_class' should probably be 'const char*'
************
ok.
************
- 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?
***********************
The PyModule_GetDict function description says that it never fails.
However, in thinking more about it, it's possible that something could
happen, i.e. bad module name, so I'll add a check for 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?
******************
I could use PyTuple_Resize.
******************
- log_message
- The parameter 'message' should probably be 'const char*'
***************
ok
*************8
- line 854, you have an fprintf() call here, probably should be
removed.
***********
oops...I'll remove that
***********
- 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.
*****************
yes. I should check that. I'll also consider strncmp.
*****************
- get_logger
- The parameter 'logger_name' should probably be 'const char*'.
***********
ok
***********
- report_progress
- The parameter 'message' should probably be 'const char*'
***********
ok
***********
install_logging.h
------------------
- The header guard (#ifndef _INSTALL_LOGGING_H) should probably be
just after the license before the other #includes.
**********
ok
**********
- 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.
***********
ok
***********
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.
**********
ok
**********
handlers.c
----------
- Similar comment as in install_logging.c above w.r.t. use of local
variable 'function'.
***********
ok
***********
- Still wondering if these should be private methods as opposed to
public methods.
******************
I think that's a good idea.
******************
Thanks,
Darren.
On 07/09/10 12:33, Ginnie Wray wrote:
Hi -
I want to get my code review started, and I still have a little
cleanup to do.
I'm going to generate my code review in two parts with the
first part the C bridge code. I haven't quite got the test code
for the C cleaned up, so I'll present that with the second part as well.
Pointer to the webrev:
http://cr.opensolaris.org/~ginnie/logging_Cfiles/
I would like to have feedback by July 23rd COB.
Thanks,
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
