Hi Jean,
I did not have a chance to review the test code. My comments below
are for all the non-test code.
Like Keith, I also have the general concern about catching the original
exception, just printing a string about it, and raising another exception.
At the very least, the original exception should be printed to the
log file using log.exception(). Even better, you can do something
similar to what ManifestParser/ManifestWriter does, which is to
return the whole original exception as the "error message". That way,
it will be easier for the caller to have the full context to figure out
what's wrong.
My specific comments below. I did this over a few days, so, if I am
inconsistent in my comments, please just let me know. :-)
----------------
usr/src/lib/install_transfer/Makefile
----------------
* line 59, lines 71-72. Since SUBDIRS is not defined or used here, anything
involving SUBDIRS is not needed.
----------------
usr/src/lib/install_transfer/__init__.py
----------------
* line 26: s/distribution constructor/transfer checkpoint/
----------------
usr/src/lib/install_transfer/transfer_cpio.py
----------------
* question about validate_file_list() function. Looks like we don't
check every single file that's specified in the list or file to make sure
all the files to be transfer exists. I think it would be good
to check that.
* line 269-277, 299-302: These 2 code blocks do exactly the same thing.
Should not have duplicate code. Also, the code block starting at 269
catches
the OSError and uses the "unsorted" file, where as the code block starting
at line 299 doesn't. Why is that?
* The functions validate_file_list(), validate_dir_list(),
validate_skip_list(),
and validate_dir_excl_list() all seem to do very similar things.
a) Handle cases were a list of files/dirs are specified verses a file
containing
the list is specified.
2) If a list of files/dirs are specified, put them into a temp file.
3) optionally, sort that file based on inode.
I wonder whether all the functions can be combined?
* line 532: can the functionalities provided by the build_file_list()
function
be replaced by the existing function find() in
usr/src/lib/install_utils/install_utils.py, plus the sort_by_inode()
function?
* line 716, line 732: log a info() message so people are
aware that they are specifying something inconsistent?
* line 772: check len(soft_list) == 0?
* line 836: __replace_doc_refs() function defined here seems similar to
the one defined in transfer_ips.py. Can they be combined?
* line 875-921: In all the @xxxxxx.setter functions, are you automatically
setting all the other variables to None. I understand from the comments
that you can't have more than one set. I think these @xxxxxx.setter
functions
should check to see whether those other variables have existing value.
If they do, raise an exception to let the caller know. Otherwise, in the
future, we might get into situation where values "mysterically" disappear.
----------------
usr/src/lib/install_transfer/transfer_info.py
----------------
* line 27: Should this comment be something along the lines of objects
stored in DOC used by the transfer checkpoint?
* I wonder whether the Software, Source, Destination, and Dir objects
can be combined into just 1 objects. To me, they all have a "label"
which can be passed in as a required arugment. Then, they have
an optional "attribute" depending on the object type. It seems
redunduct to have multiple object definitions that are so similar.
* line 217, 223-224: Looks like ssl_key and ssl_cert is never used.
I don't think they needed here.
* lines 363: Just curious, why do you need to use "getchildren()" to
get the value for facet? In line 343, you set the "facent_name" using
"element.text=....". In line 383, you set "prop_name" using
"element.text=.....", but in line 401, you don't need to use getchildren()
to get the value for "prop".
* line 363-364: Do you need to make sure "facet" is a valid value
before creating a Facet() with it as an argument. The __init__()
function of the Facet object doesn't check it's argument either, so,
it would be easy to let something invalid slip in.
* lines 481-539: From the check in line 541, I realized that only
one transfer spec is allowed. I think it is very inefficient to
go through all these code just to find out that you specified more
than you should. Would it be more efficient to do something like this?
def __init__(self, spec_type, list)
where:
- spec_type either one of INSTALL_FILE, INSTALL_DIRS, SKIP_FILE_LIST,
EXCL_DIR_LIST, MEDIA_TRANS
- list is the file name or list of files/directories
The the to_xml() can deal with just 1 spec type.
* line 590-652: this from_xml() function can potentially create an object
that the to_xml() can not handle. This function can set all 4 of
file_list, dir_list, skip_file_list and dir_excl_list, but only 1 of these
is allowed in the to_xml() call.
* The CPIOSpec() object doesn't seem to deal with the "media_transform"
variable
at all. Is that an oversight?
* line 666: value for purge_history is not saved in the to_xml() call.
Is that expected?
* line 724, this function doesn't transfer "app_callback" and
"purge_history"
to xml format. Is that OK?
* line 897: I see that "publisher_name" is not required. Is it OK to
have "None" as a publisher?
* line 936: origin_name is also not required. Is it OK for the Origin
object
to have None as origin?
* line 975: ditto comment about mirror_name being None.
* Looks like Publisher, Origin and Mirror objects are very similar
except the label value. Can they be combined into 1 object and you pass
in what label you want that object to be in the __init__
----------------
usr/src/lib/install_transfer/transfer_ips.py:
----------------
* For all the exceptions, if you use self.logger.exception() instead of
self.logger.critical, you will be able to dump
the full trace to the log file, which is more useful for debugging, I think.
* line 86: should this line be deleted? :-)
* line 92: I think valid strings for "self.action" should be pre-defined
somewhere. So far, from reading the code, I think we have "create" and
"use-existing", which are both hard coded everywhere.
* line 108: probably no need to save "dry_run" value.
* line 211: I think a "else" is needed to take care of the case
where self.completeness is not IMG_TYPE_ENTIRE and IMG_TYPE_PARTIAL
* line 212-213: Nit: is_zone is not really checked. It's just printed here.
Should this be in the "validate_input" function?
* line 364, 388, 411: Is it better to print this before the "if"
statement? Also,
include the list of packages you would be installing? That way,
even if dry_run mode, one can still see that you are going to install
some packages
* line 456: The get_ips_api_inst() function doesn't seem to check whether
it is dry run or not before doing the "create". Should it?
* line 550: I think also need to check len(soft_list) == 0
* line 744: also check len(src_list) == 0?
* line 765-810: In the @xxxxx.setter functions that sets the different
variables for the TransferIPSAttr object, not only the named variable
is set. Other variables in the object is set as well. For example, in
the @pkg_install_list.setter function, I think it should only "touch"
the value for _pkg_install_list variabl. However,
_pkg_uninstall_list and _purge_history variable are set to None. If
they have some existing values, those would be lost.
------------
usr/src/lib/install_transfer/transfer_p5i.py
------------
* lines 128-131: Why do we want to append "None" to the list? Isn't it
better to just to have an empty list?
-------------
usr/src/lib/install_transfer/transfer_svr4.py
-------------
* line 71: why need to save dry_run value?
* line 156: No need to call self._cleanup()?
* line 218, line 257: It would be useful to have this log message before
the "if" statement so we know what packages are to be installed even for
dry_run.
* line 337: check len(soft_list) == 0?
* line 361-362: shouldn't this check be after line 353? Also check
len(src_list) == 0
* line 486: why set _pkg_uninstall_list = None? Should only set
_pkg_install_list.
* line 497: why set _pkg_install_list? should only set
_pkg_uninstall_list here.
Thanks,
--Karen
On 09/30/10 10:18 AM, jean.mccormack wrote:
I'd like to request a code view for the Transfer Module checkpoint
for the CUD project.
The webrev is here:
http://cr.opensolaris.org/~jeanm/transfer_module/
Please send comments by 10/12. I realize there are now 3 big code
review outstanding. If you need more time please contact
me and I can be flexible.
Here's the bookkeeping details:
pep8 runs cleanly on all files
pylint results:
__init__.py 10.0
transfer_cpio.py 8.26
transfer_err.py 10.0
transfer_info.py 9.38
transfer_ips.py 8.7
transfer_p5i.py 9.74
transfer_prog.py 8.37
transfer_svr4.py 8.55
test coverage results:
transfer_cpio.py 82%
transfer_ips.py 76%
transfer_svr5.py 86%
transfer_p5i.py 94%
transfer_err.py 94%
transfer_prog.py 87%
Jean
_______________________________________________
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
