Hi karen -
thanks for the review. My comments below.
ginnie
On 07/23/10 12:24, Karen Tung wrote:
Hi Ginnie,
Here are my comments on the C files.
usr/src/lib/install_logging/handlers.c
-----------------------------------------
lines 65-70: The value for pFileDict won't be used until line 86. Why not
move these few lines down closer to where it will be used.
ok.
lines 166-178: This block does not need to be in an else statement.
The "if" will catch the failure case.
Right. I'll fix that.
The create_httphandler() and create_streamhandler() functions in this
file will not have the corresponding back-end support in Python. Do we
really want to have the code here now?
The streamhandler is supported in logging now.
Sanjay and I discussed the httphandler, and decided to go ahead and put
it in because it sounds like the other functionality will be coming soon.
usr/src/lib/install_logging/install_logging.c
-----------------------------------------------
lines 200-201: To improve readability of the code, I think it is better
to give more meaningful names to these variables. What about
pModuleLogging and pModuleLogsvc
That's a good idea. thanks.
line 221: do we need to check whether "pDict" is NULL before using it?
yes.
line 254: This line will get executed even if there's no error. According
to Python document, it will cause a fatal error.
Sanjay, Keith and I discussed this and decided for set_logger_class and
get_logger to use PyErr_Print. For the others I use PyErr_Clear.
line 276: The Python version of this function allows users to pass in
a "source". Are we not going to allow that functionality in the C version?
Yes...I sent this out and then modified the python version. I've since
updated this.
line 487: I think it is more descriptive to name this function
"set_log_level"
ok. I can change that.
line 512-513, 660-661: Same comment as above about the 2 variable names
I'll change that for all of them.
line 854, 949: debug messages should be removed.
Missed those.
usr/src/lib/install_logging/install_logging.h
-----------------------------------------------
line 44: socket handler is never used. Should not be defined.
I'll remove it
Thanks!
Thanks,
--Karen
On 07/ 9/10 11:33 AM, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
