Hi Jean, Thank you for the code review, please see my response inline.
Jean McCormack wrote: > Here's my comments: > > General: what if dc_log = logging.getLogger(DC_LOGGER_NAME) returns null > or generates an exception? > > It will never return null or generate an exception. The way the Python logging works is that if DC_LOGGER_NAME doesn't already exist for some reason, it will create a new one for you. If it already exist, it will return you a handle to that instance. If it really doesn't exist, the new one created will use the default logging settings. Because of this, I can get away with not having to pass the handle around, and not checking the return value, because it will never be null. In fact, in some place that I read, it says that they make it behave this way so we don't need to pass the handle around. > DC_checkpoint.py: > line 280: Shouldn't that be if (log_handler != None): > Yes, it should be. Fixed. > Question: Should all print statements now go to dc_log? Or > only select ones? > I carefully examined all the functions in DC_checkpoint.py. All the ones that are called after the DC log is initialized will go to the dc_log. All the ones that are called before the DC log is initialized just go to the screen. The DC logging can't be setup until the work area is setup. So, all the functions that involve creating, checking the work area and stuff won't be able to do logging to the DC log. > finalizer_checkpoint.py: > In other code you modified the shell_cmd calls to include logging. Do > you want to > do that here too? > Yes, good idea. Added. > finalizer_rollback.py: > ditto > Added as well. > line 66: that should be 9. One of the "length" is due to the program > itself, not an arg. > > > Fixed. > post_bootroot_pkg_image_mod: > Please add the usr zlib compression algorithm to the header comments > Added comment to header > finalizer.py: > line 609: Should there be a space after signal and before " ? > Yes, space added. > transfer_mod.py: The assumption here is that you won't be logging via > the liblogsvc module > and via this logging module. Correct? > If a logging handler is passed in, then, the logging handler will be used for logging. If no logging handler is passed in, then, everything will be done as before, which is using the liblogsvc module. Thanks again for your comments. --Karen > Jean > > > > Karen Tung wrote: > >> Hi, >> >> Please do a code review for changes to fix the following bugs: >> >> 3341 stop_on_error doesn't seem to work for finalizer scripts >> 3354 Logging of the DC app >> 3482 Return status from DC needs to reflect errors encountered >> 3417 /var fails pkg verify >> 3536 Change constructor to use lzma for non-usr archives on primary live CD >> 3460 Remove attrs and tags in <packages> section in manifest >> >> webrev: >> >> http://cr.opensolaris.org/~ktung/dc_logging/ >> >> Thanks, >> >> --Karen >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
