Hi Jack,
Please see my response inline. I removed items that I have
no further comments on.
On 03/29/11 20:50, Jack Schwartz wrote:
Hi Karen.
Thanks for taking time to review.
On 03/23/11 01:00 AM, Karen Tung wrote:
Hi Jack,
Here are my comments for all the files in group D. If a file is not
mentioned,
that means I have no comment on it.
-----------------------
usr/src/Makefile.master
------------------------
Line 109-110: I don't see the "derived_manifest_test" directory in the
list of files for code review. Is it supposed to be there?
Makefile.master sets up directories in the proto area, not in source
files.
Most functionality have the "tests" directory as a subdirectory
in the area where the code is. So, I am surprised to see a special
directory
for tests.
Some library modules have tests in the proto area, for example
./solaris_install/engine/test
./solaris_install/engine/test/engine_test_utils.py
./solaris_install/engine/test/engine_test_utils.pyc
./solaris_install/target/test_td.py
./solaris_install/target/test_td.pyc
There doesn't seem to be a standard way of naming though (in a test
subdir, not in a test subdir,...)
Tests are generally not delivered to the proto area. The script that runs
all the unittests in the slim_source gate looks for those tests in usr/src
itself. So, it is unnecessary to deliver the tests to the proto area.
I can't speak for the test_td.py file. The engine_test_utils files are
delivered
to the proto area because it contains functions that are used by tests
from many other CUD components.
"derived_manifest_tests" seems a bit long too. I'll rename
"derived_manifest_tests" (which is under the AI checkpoints directory)
to be "test" . This will also be more generic, so if others want to
add tests for their checkpoints they can put them there.
Like test_utils, I'll just store the tests in the proto area and not
add them to a test package, unless someone says they should go in the
install-tests package.
I will also store other unit tests in the proto area as well.
- line 48-80: is there a reason for not using InstallLogger(), which
should
be used for all install code.
Yes, there is. aimanifest is intended for use mainly by the derived
manifests module (DMM). The DMM takes output from the scripts it runs
(and thus the commands those scripts run) and logs them. To do so
again in aimanifest would result in double-logging.
As we discussed in person, the InstallLogger() can be used.
However, the DM spec calls for aimanifest to log to a logfile of its
own, as specified by the environment variable AIM_LOGFILE. This is
what I set up.
As I scrutinize this module, however, I see that there are several
places where error logging (as opposed to just error printing) is
missing. This will be changed before code review round 2.
- line 98-121: instead of having a function dedicated to creating
the usage string, the fix part of the string can be
defined as a constant, and all you had to do is to input the
program name as you have them.
Currently the usage shows the command name in several places, so I'd
like to leave the implementation as is.
The command name is shown in several places. However, the usage()
function is
called only once to create the usage_str that's passed around.
My comment is to not use a usage() function at all.
Instead, at the place where you call the usage function to create
the usage_str, create the usage_str by substituting in the
command name in-place.
That said, I am not extremely advocating that you make this
change. So, it is up to you to do it or not.
- For all the exceptions, you have a function called handle_error() to
deal with the exception. I guess you do that because you don't want
the user to see the full trace back. However, there's useful
information
in the traceback that should be captured.
handle_error() is called for specific exceptions due to expected kinds
of errors, such as file not found or mim_errors. Unexpected
exceptions will go through the normal
bubble-up-to-the-commandline-with-traceback handling. In this case,
tracebacks will be output to stderr, and the DMM will capture and log
the errors properly.
Is there any harm to showing the full traceback for file-not-found or
MimErrors? Since they also cause the program to
exit with non-success return code, why not let the full trace be shown?
After the discussion we just had with Ethan regarding logging, I think
all debugging info and errors will now go
to the log file, right? So, all these tracebacks will go to the log
file, and a non-zero return code will
be returned regardless what kind of trace it is.
Thanks,
--Karen
The most appropriate place
is the log file, I think. Instead of logging just the error string
from the traceback to the log file in the handle_error() function, it
would be useful to just call AimLog.exception() inside the "except".
That way, the full exception will be captured. Then, the handle_error()
function can still be used to hide the exception from the user.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
