Hi Keith and all other code reviewers -
I went through all of the back and forth between you and Jean to
make sure I didn't miss any thing, and I thought I would use your
original to give you feedback on the overall code. I hope that makes it
easier for you to review. See below.
I have, as well, input the comments from other reviewers.
The final code review is posted at:
http://cr.opensolaris.org/~ginnie/trans_final2/
I would like to plan to putback by tomorrow, late afternoon.
Thanks,
ginnie
On 10/11/10 18:29, Keith Mitchell wrote:
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.
Hi Jean,
There's an impressive amount of code here, so pardon the delay in
responding. I've got a fair number of nits and straightforward
Pythonizations below; the only functional issue I have with the code is
the error handling. In particular, I feel the error handling was a bit
too excessive in the use of try/except clauses - a lot of exceptions
were caught and then a new, more generic exception was raised. The
details are below, but I feel a lot of it could be stripped out with no
harm to the resultant code - it's perfectly fine to let exceptions
propagate up a few levels without catching them.
My rule of thumb is to only catch if (a) the catching code can resolve
the problem, or (b) the catching code can add additional context (either
by logging additional data for later debugging, or by wrapping the
exception in a more specific exception that adds details/messages about
the context in which the original exception was raised, with the goal of
either providing the end user a better idea of how to resolve the issue,
or providing a more discrete exception for a caller to parse out).
I haven't had a chance to look at the test code yet; while I still want
to take a (less detailed) look, I wanted to send this out so that it
doesn't sit too long in my drafts folder.
Thanks,
Keith
General:
Since the files are going under solaris_install/transfer, I think it
might make more sense to remove the "transfer_" from the .py files.
import solaris_install.transfer.transfer_ips
seems redundant compared to
import solaris_install.transfer.ips
(The class names themselves, e.g., TransferCPIO, are fine)
Done
usr/src/lib/Makefile: Nit: Alphabetize
Done
system-library-install.mf:
Should this actually be in system-install.mf? That's where the current
transfer module is.
You and Jean decided ok as is.
transfer_cpio.py:
64: This variable might be more flexible as a tuple of ("-p", "-d",
"-u", "-m")
I've modified this to be more flexible, giving the code the ability to
append the args to the cmd list.
99-107: It seems to me that this scenario might be better handled by
something akin to the following:
except SourceNotExistError:
# _parse_input should raise this for cases such as "self.src and not
os.path.exists(self.src)"
return self.DEFAULT_SIZE
Note that the 'else' clause is purposely left out - the
TransferUnknError raised here can't provide a better reason for the
failure, so there's no reason not to simply let the original exception
propagate.
119: Use "partition" instead of "split" (partition won't cause an error
if there happens to be a line in the file not of the form "XXX=YYY" at
some point in the future)
implemented
115-124: I think this should be done prior to the 'for' loop
gone w restructuring
155: Nit: Would it make sense to use self._cancel_event for consistency
with other checkpoints that don't define a custom cancel() method?
implemented
159: I'm not sure what the reason is for this method - can you explain?
Will other checkpoints feasibly need something similar (i.e., should
there be something more global in the engine or logging module)?
removed
184, 188: General: When logging from except clauses, use
logger.exception() rather than logger.error/critical, as
logger.exception automatically dumps traceback data for the current
exception. However, see next comment
A lot of the exceptions have been remove in order to let the exceptions
propagate up, so I believe I've addressed this where necessary.
183-189: I think it might be best to simply propagate the error (putting
cleanup steps in a 'finally' clause and removing both 'except' clauses).
The engine will take care of logging the uncaught exception. In
particular, re-wrapping the error on line 189 causes the loss of useful
data from the original exception, and I don't see any value added
specifically by the TransferUnknError here.
implemented
209-210: As this is cleanup code, I think it should be wrapped in
try/except so that an issue with an unlink doesn't cause an exception to
get raised (e.g.; if an exception was raised, triggering cleanup, and an
exception happened in the cleanup, the original - and more important -
exception would get lost)
implemented
235: Some comments explaining what's happening here would be invaluable.
gone
248: I think this could just be "if num_spec != 1:"
gone
253, 319, 388, 437: I'm generally averse to python functions that accept
either a string or a list as a parameter, as it results in code that
requires this sort of type-checking, rather than taking advantage of
Python's dynamic typing a bit better.
If having separate calls for lists and strings doesn't make sense here,
a better alternative is to flip the comparison: check for "if
isinstance(file_list, basestring):", and let the else clause assume an
iterable (list/tuple/etc.) - the idea being that in this fashion, if
dir_list is a tuple, or a "list-like" something or other, the function
still works Pythonically.
266-273: Why not sort first in memory, then write to file? Then you
don't have to write to file twice.
gone
291, elsewhere: I think Drew had a good suggestion here, but given the
behavior of os.path.join, this could simply be:
fname = os.path.join(self.src, file_list)
os.path.join, upon encountering any arg that is an absolute path,
disregards all prior arguments, so there's no need to if/else this.
implemented
427: The validate_xxx_list functions all look very similar; I imagine
they could be readily combined, or use a common sub-function.
gone
526: Seems more reasonable to just let the OSError bubble up.
gone
544: Should log this and propagate the original error; or capture the
original error string, so that there can be some determination of why
the file couldn't be opened.
Restructured per Drew's feedback
564: If line 559 raised an OSError from os.lstat, this line will raise a
NameError because nothing was set. Suggest removing the try/except block
(it should be nearly impossible for something to occur there anyway,
given that line 550 succeeded if we reach this point).
done
604: Line appears to be missing an argument.
fixed
649: log or stash the original exception
done
654: Propagate original exception
done
667: Would rather not see yet another "run/exec/cmd" function in our
source code. Given the current single use case for this function, simply
embed the check_call in _transfer - would even be fine without the
try/except clause, I think.
Jean's email response about coming up with something that everyone
agrees on addresses this issue
704: Use "self.logger.info("Transferring files to %s", self.dst)"
As a general rule, for logging statements, use formatting string
operations and pass in the formatting arguments as separate parameters.
This allows the logging module to defer the (relatively time intensive)
string formatting operation until it determines if the logging call will
actually occur or not (based on the log levels).
done
While it's only really important for code that is run frequently (in a
loop processing a large amount of data, for example), it's a good
practice to get into.
754-760, 960-966: These are init'ed in AbstractCPIO - is there a reason
to reset them here?
gone
transfer_err.py:
Philosophically speaking, i don't see a need for these exception
classes. Uses of "TransferValueError" should definitely be directly
replaced with the basic Python ValueError, which callers would
reasonably expect from a function if they pass in invalid parameters.
The rest of the exceptions defined here seem to indicate *where* the
exception came from, rather than *what* the exception was - the latter
being far more valuable to a caller or end-user. The engine already
knows what checkpoint is running - and the stacktrace data from an
exception will further make that clear. In terms of resolving an issue -
either finding a programming bug that raised an exception, presenting
adequate information to the user to fix a permissions issue, or
programmatically recovering from an expected error case - it's easier to
use an exception that matches what happened (IOError - bad permissions /
file not found; TypeError - Function can't handle objects of type XYZ;
etc.).
gone
transfer_info:
45: ret_value needs to get set whether or not conv_str is None (that is,
what happens if conv_str = "foobar"?)
implemented
47/49: Nit: just compare conv_str.lower() == "false"
(or conv_str.capitalize() == "False" - capitalize will ensure that
*only* the first letter of each word is capitalized)
That's ok. That's what we need
58-59: The "SOFTWARE_" prepended is redundant - just use "LABEL" and
"NAME_LABEL"
Ditto for the equivalent items in Destination and Dir classes.
The meeting of the minds was leave it, from my reading of the email exchange
119: Nit: As a one-liner: "return element.tag == Software.LABEL". Ditto
for the other classes defined here.
done
301: See above note on capitalize()
Noted
482: Just to clarify, the expected action here is that if self.file_list
== self.DEF_FILE_LIST and if self.file_list *can't* be accessed, then a
self.dir_list action would override it? Or should that be an error?
This was addressed in Jean's response
552: Flip the if/else such that the comparison is "if
isinstance(file_name, basestring):" (see above)
implemented
604-639: These lists can be stored in memory, right? Would it be better
to do so? A temporary file seems prone to issues across pause/resume,
particularly if the system is rebooted in between.
775-777: DTD validation will ensure that action is never anything but
"install" or "uninstall", correct?
the answer was yes.
1007: I know this does something in vi, but it seems odd to have it in
our source base.
removed
transfer_ips.py:
73: My understanding of gettext is that gettext.install should *only* be
called by an application, never by a module - modules should use
something like:
_ = gettext.translation
gettext.install() modifies the global namespace, so running it will
affect all imported modules - which means this line here will override,
or be overridden by, the call to gettext.install() that will come from DC.
Similarly, I don't think a checkpoint should set locale.LC_ALL globally.
Jean addressed this
80-83: Please add comments for these values
done
86: Remove commented code
done
110: Nit: (Same as prior nit) - would it make sense to use
self._cancel_event for consistency?
implemented
134: Can you explain what this math is doing here? It appears to be that
each pkg is assumed to be "roughly" the same size based on a
pre-generated average, and the number of packages given is correlated
against that?
Jean addressed this
139-140: Since get_size() appears to only be used here, could
get_size/distro_size be combined into a single @property called
distro_size? Use an internal self._distro_size variable to cache the
results of the work currently done in get_size(); if the cache is set,
return its value; if not, calculate it, set it, and then return the value.
Agreed no change needed
153: Same prior question regarding whether there's a deficiency in the
logging module or engine that needs to be resolved.
gone
178-184: See comments on lines 183-189 of transfer_cpio.py
implemented
201/202: Use "self.logger.debug("Destination: %s", self.dst)
695, 711, 725, 761, 835: Similar comments
implemented
215-219: Use set operations:
not_allowed = set(["prefix", "repo_uri", "origins", "mirrors"])
image_args = set(self.image_args)
overlap = a & b:
if overlap:
# error
implemented
217-219: Malformed format string
fixed
222-3: Quick nit:
self.prog_tracker = self.img_args.get("progtrack", self.DEF_PROG_TRACKER)
Should achieve what you're going for, I believe.
done
228: A recent change in pkg(5) made it so that publishers with installed
packages could never be removed - removing them would actually simply
*disable* them. Will that cause problems with this approach, i.e.,
removing an existing publisher on line 310, then adding it back in (with
potentially different origin/mirrors) on line 320/324?
Would it be better to iterate through all existing publishers; updating
any that match what the specified preferred/alternates are, disabling
any that don't match, and then adding any new ones necessary?
you and Jean discussed and determined no change
269, 276, 296: Use logger.exception
Letting these percolate up to the engine.
281: This would be more "natural" as a for/else clause:
for pub in pub_list:
if pub == self._publ:
# do stuff
break
else:
# No "break" clause execute -> Did not find preferred publisher
# set the new preferred publisher
This doesn't work. They need to be two separate checks or it fails.
394: pop() is a function, so the arg should be in parentheses, not
brackets. pop() also removes the item from the dictionary, which may or
may not be desired (e.g. if something fails, but it's recoverable, and
this code executes again, "recursive_removal" will be gone - it may be
desirable to instead create a copy of args and use that for this section
of code)
done
An empty dictionary will work just fine with **, so if args is always a
dictionary (or possibly None - in which case, add an "if args is None:
args = {}" chunk of code somewhere), then 391-401 can be combined into:
recursive = args.pop("recursive_removal", False)
self.api_inst.plan_uninstall(pkg_uninstall, recursive, **args)
438-439: Code can be removed
done
451: Will stacktrace if self._origin is None. (some_list[1:] will never
be None - if there are no items there, it will return an empty list).
line 451 could probably just be removed, and line 452 could be part of
the 'if' block from line 449.
done
456: What happens if self.action is neither "use_existing" nor "create"?
validation checks the values now
504: Include the URI in the exception (or propagate the
InvalidDepotResponseException if it has a suitable message
Allowing to propagate up
507: I don't know if this message will make sense to an end user - how
would they end up in this situation, and how might they resolve it? The
API Errors from pkg.client.api_errors seem fairly verbose - I think it
would be possible to simply propagate them, unless we have additional
context to add to the message, or have a way to recover from the exception.
decided ok as is
569: What is this clause catching? The message seems rather specific to
a failure at line 563, and I imagine it could be constrained to a single
exception class.
These errors are now being propated up
575-578: If im_type.zone is a bool, simply set "self.is_zone =
im_type.zone". If it's something else, use "self.is_zone =
bool(im_type.zone)"
fixed
642: Nit: Initialize value_str to an empty string, and lines 646-648
aren't needed.
No longer in our code
712, 726: A more specific class than "Exception" would be preferred here.
done
683: In looking at the complexity of the branching, I wonder if a lot of
the publisher related code, here and elsewhere, would be simplified by
having a single list of publishers, stored in "search" order. The first
item in the list would be what's known as the "preferred" publisher, and
the rest are additional publishers, in order of precedence. This would
seem to map more naturally to how the publishers on a system are laid
out these days (preferred is nothing more than a way to say "search this
publisher first").
kemit...@kemobile-work->~ 0 $ pkg publisher
PUBLISHER TYPE STATUS URI
opensolaris.org (non-sticky, preferred) origin online
http://ipkg.sfbay/dev/
contrib origin online
http://pkg.opensolaris.org/contrib/
extra origin online
http://ipkg.sfbay/extra/
kemit...@kemobile-work->~ 0 $ pfexec pkg set-publisher
--search-before=opensolaris.org extra
kemit...@kemobile-work->~ 0 $ pkg publisher
PUBLISHER TYPE STATUS URI
extra (preferred) origin online
http://ipkg.sfbay/extra/
opensolaris.org (non-sticky) origin online
http://ipkg.sfbay/dev/
contrib origin online
http://pkg.opensolaris.org/contrib/
ok as is.
745: Nit: The "+" and the "\" at the end aren't strictly necessary (The
"\" is implied since there is an unclosed parentheses; the "+" is not
required for concatenation of inline string constants)
(Saw this in a few places, truly not a big deal, just thought I'd point
it out for future reference)
removed
837-841: Nit: Enclose in an "if
self.logger.isEnabledFor(logging.DEBUG):" clause, for reasons similar to
prior logging comments.
ok as is
869-871: Use "elif"?
done
881: Initializing self.args to an empty dictionary would remove the need
for this type of logic.
done
transfer_p5i.py:
77-82: Remove try/except clause, and let exceptions propagate?
done
67, 87: You've popped the first publisher twice - was that intended?
Jean responded to this
transfer_prog.py:
70: Would os.statvfs provide the necessary data, rather than spawning a
subprocess?
See:
http://stackoverflow.com/questions/787776/find-free-disk-space-in-python-on-os-x/787832#787832
done
77: Needs a "stderr=subprocess.PIPE" argument so that errors aren't
dumped to the console
80: Should check that the command succeeded, otherwise this may error
out with the IndexError seen on 82.
82-83: Propagate original error
went away with the statvfs change
92: Seems odd to override the value for self.endpct that was set on line
60 in __init__
(Or should that 2nd equals be a minus sign perhaps?)
fixed
101: When would this occur?
removed
103: This looks odd as well - should the second equals sign be a minus
sign?
fixed
118: Could this be moved up to line 98 as the condition on the "while"
statement?
Did not change
120: This should be a tweakable attribute (i.e., in __init__, set
self.sleep_for = 2)
changed
transfer_svr4.py:
87: Nit: Just use "for pkg_install in self._tr_pkg_install:"
99: Could use more specific error handling and messaging.
both gone with restructure
101: Move this comment to the docstring
done
107: Have _parse_input raise a specific exception for this scenario, and
catch that; see comment regarding lines 99-107 of transfer_cpio.py
done
103-133, 162-190: This seems to overlap with the same functions in prior
files; would a common meta-class be worthwhile, defining these basic
things?
Decided as is was fine.
154-160: See comment regarding lines 183-189 of transfer_cpio.py
done
210/212: Nit: Use "for pkg_install in self._tr_pkg_install:"
(Ditto for line 249/251)
gone w restructure
232-245: Use something like:
while pkg_proc.poll() is None:
self.check_abort()
# possibly sleep for a little bit
stdout, stderr = pkg_proc.communicate()
# log stdout, check pkg_proc.returncode != 0, etc.
The problem with using pkg_proc.poll() is None is when the command fails
immediately. the poll() value is set to one and never drops into the
while loop. If this happens, the error isn't logged.
The way I understand communicate, it reads all of the output and waits
for the child process to exit before returning. I saw quite a few
examples using readline, and since there could be a lot of output with
pkg add/remove, I thought it would work better.
245: This seems like it will add a *lot* of output at the info level,
which is probably not useful. Perhaps log at the debug level, or only
log if the pkgadd command failed?
yes. That's been addressed
248-276: Seems like this could be combined with the "install" block by
doing something like:
if self._tr_pkg_install:
pkgs = self._tr_pkg_install
base_cmd = self.PKGADD
elif self._tr_pkg_uninstall:
pkgs = self._tr_pkg_uninstall
base_cmd = self.PKGRM
for pkg in pkgs:
# prep work
cmd = [base_cmd] + arglist
if self.datastream:
cmd.append(datastream)
pkg_proc = subprocess.Popen(cmd, ...)
# continue as usual
322: Line not needed, given line 327
335: No need to concatenate there
345, 355: Needs more specific exception(s)
414-425: Seems like this could be condensed similarly to lines 232-245.
Since this doesn't need to be interruptible, it could be squished even
further into:
stdout, stderr = pkg_proc.communicate()
if "package datastream" in stdout:
# datastream pkg
gone with restructure
428-443: Seems like it would be possible to remove this limitation, by
correlating each pkg with its own set of arguments (datastream or
non-datastream, or uninstall)
packages will be either all datastream or all non-datastream.
477-497: Might be worthwhile to simply change these to normal
attributes, and then simply have the _parse_input function raise an
exception if both are set?
These are all gone with restructure
499-577: Seems like this duplicates a lot of the code in the prior class
- perhaps some reorganization is in order?
With the restructure, there are subtle differences that would make it a
bit of a pain.
_______________________________________________
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
