Karen,
Thanks for the review. Comments are inline.
Jean
On 10/12/10 11:36 AM, Karen Tung wrote:
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. :-)
Ginnie and I are going to talk with Keith when we get to the error
stuff. I think we'll figure out something nice.
----------------
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.
OK
----------------
usr/src/lib/install_transfer/__init__.py
----------------
* line 26: s/distribution constructor/transfer checkpoint/
how embarrassing.....
----------------
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.
Actually we don't need to. If they don't exist it's not a deal breaker
and the overhead would
be overwhelming. We do print out a warning just in case though.
* 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?
Both should have caught the OSError. This code has been revamped with
the restructuring changes.
I believe the code duplication goes away with 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?
yes. They have a lot of similar code which can be pulled out and then
the slight differences performed.
* 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?
Not sure. But it's worth looking at.
* line 716, line 732: log a info() message so people are
aware that they are specifying something inconsistent?
sure.
* line 772: check len(soft_list) == 0?
Ginnie is putting that into the init method as discussed so I took it
out of here. With rev 2 her changes will be there.
* line 836: __replace_doc_refs() function defined here seems similar to
the one defined in transfer_ips.py. Can they be combined?
Darren is putting it into the DOC code. Hopefully soon.
* 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.
These are all gone now. With the restructuring what was being protected
against isn't even possible.
----------------
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?
Yeah. It should be something different.
* 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.
I think it's easier for the user to specify Source() and Destination()
conceptually
rather than Something(LABEL, attribute). In fact, I think the dc spec
file also
has some objects looking very much like these and we could make life really
difficult for the user.
* line 217, 223-224: Looks like ssl_key and ssl_cert is never used.
I don't think they needed here.
correct. Nice catch.
* 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".
I'm guessing it's a bug. Will need to look at it.
* 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.
That's done in the transfer_ips code. Since a facet value can come in
via attributes, manifest or DOC it makes sense to only
do this checking in one place.
* 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.
This is actually all gone with the restructuring. Much simpler.
* 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.
Changed and gone.
* The CPIOSpec() object doesn't seem to deal with the
"media_transform" variable
at all. Is that an oversight?
Nope. Sarah didn't want it in the manifest so it never gets written out
or read.
CPIOSpec itself can take a media_transform but that is only for the DOC
interface, not the manifest.
* line 666: value for purge_history is not saved in the to_xml() call.
Is that expected?
Same reason as above.
* line 724, this function doesn't transfer "app_callback" and
"purge_history"
to xml format. Is that OK?
Same reason as above.
* line 897: I see that "publisher_name" is not required. Is it OK to
have "None" as a publisher?
It's OK. IPS doesn't require a publisher name anymore.
* line 936: origin_name is also not required. Is it OK for the Origin
object
to have None as origin?
Yes. If origin is not specified we have a default.
* line 975: ditto comment about mirror_name being None.
yes.
* 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__
I'd prefer not. I really would like to keep things obvious for the user
and allow flexibility if we need it in the future.
----------------
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.
Will change.
* line 86: should this line be deleted? :-)
yes. You know, everyone caught this. At least people are paying attention.
* 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.
Sounds like a good idea.
* line 108: probably no need to save "dry_run" value.
Except that it's nice not to have to pass it around but rather to keep
it in an attribute.
* 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
Yes. The manifest forces full or partial but if you're just writing
through the DOC you could in theory put anything yu want in there.
* line 212-213: Nit: is_zone is not really checked. It's just printed
here.
Should this be in the "validate_input" function?
It's taken care of in parse_input. Forced to True or False.
* 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
If I put it before the if it prints even if you have no packages to
install. Definitely not what we want.
Could do something like this:
if pkg_install:
self.logger.info("Installing packages: " )
if not self.dry_run:
That would give what you want.
* 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?
The check is not to call get_ips_api_inst if dry_run.
* line 550: I think also need to check len(soft_list) == 0
Ginnie is adding that code in the init method per one of our dc meetings.
* line 744: also check len(src_list) == 0?
The source can be none. That implies you want to leave the source as it is.
* 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.
Like CPIO, this code will be gone.
------------
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?
Ignore this code. It goes away with restructuring.
-------------
usr/src/lib/install_transfer/transfer_svr4.py
-------------
* line 71: why need to save dry_run value?
Same as for IPS.
* line 156: No need to call self._cleanup()?
Keith has asked for some changes to this code, while doing this we'll
look at if cleanup should be called.
* 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.
Similar case to IPS. Seems like a good request so we may just break up
the if into 2 and put the log message there.
* line 337: check len(soft_list) == 0?
Same answer as for IPS
* line 361-362: shouldn't this check be after line 353? Also check
len(src_list) == 0
In this case, I think you do need a source. Ginnie will need to answer
that. Not sure it matters really where
if len(src_list) > 1 check is done.
* 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.
Again, killed code.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
