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)
usr/src/lib/Makefile: Nit: Alphabetize
system-library-install.mf:
Should this actually be in system-install.mf? That's where the current
transfer module is.
transfer_cpio.py:
64: This variable might be more flexible as a tuple of ("-p", "-d",
"-u", "-m")
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)
115-124: I think this should be done prior to the 'for' loop
155: Nit: Would it make sense to use self._cancel_event for consistency
with other checkpoints that don't define a custom cancel() method?
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)?
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
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.
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)
235: Some comments explaining what's happening here would be invaluable.
248: I think this could just be "if num_spec != 1:"
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.
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.
427: The validate_xxx_list functions all look very similar; I imagine
they could be readily combined, or use a common sub-function.
526: Seems more reasonable to just let the OSError bubble up.
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.
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).
604: Line appears to be missing an argument.
649: log or stash the original exception
654: Propagate original exception
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.
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).
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?
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.).
transfer_info:
45: ret_value needs to get set whether or not conv_str is None (that is,
what happens if conv_str = "foobar"?)
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)
58-59: The "SOFTWARE_" prepended is redundant - just use "LABEL" and
"NAME_LABEL"
Ditto for the equivalent items in Destination and Dir classes.
119: Nit: As a one-liner: "return element.tag == Software.LABEL". Ditto
for the other classes defined here.
301: See above note on capitalize()
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?
552: Flip the if/else such that the comparison is "if
isinstance(file_name, basestring):" (see above)
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?
1007: I know this does something in vi, but it seems odd to have it in
our source base.
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.
80-83: Please add comments for these values
86: Remove commented code
110: Nit: (Same as prior nit) - would it make sense to use
self._cancel_event for consistency?
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?
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.
153: Same prior question regarding whether there's a deficiency in the
logging module or engine that needs to be resolved.
178-184: See comments on lines 183-189 of transfer_cpio.py
201/202: Use "self.logger.debug("Destination: %s", self.dst)
695, 711, 725, 761, 835: Similar comments
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
217-219: Malformed format string
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.
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?
269, 276, 296: Use logger.exception
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
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)
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
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.
456: What happens if self.action is neither "use_existing" nor "create"?
504: Include the URI in the exception (or propagate the
InvalidDepotResponseException if it has a suitable message
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.
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.
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)"
642: Nit: Initialize value_str to an empty string, and lines 646-648
aren't needed.
712, 726: A more specific class than "Exception" would be preferred here.
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/
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)
837-841: Nit: Enclose in an "if
self.logger.isEnabledFor(logging.DEBUG):" clause, for reasons similar to
prior logging comments.
869-871: Use "elif"?
881: Initializing self.args to an empty dictionary would remove the need
for this type of logic.
transfer_p5i.py:
77-82: Remove try/except clause, and let exceptions propagate?
67, 87: You've popped the first publisher twice - was that intended?
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
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
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?)
101: When would this occur?
103: This looks odd as well - should the second equals sign be a minus sign?
118: Could this be moved up to line 98 as the condition on the "while"
statement?
120: This should be a tweakable attribute (i.e., in __init__, set
self.sleep_for = 2)
transfer_svr4.py:
87: Nit: Just use "for pkg_install in self._tr_pkg_install:"
99: Could use more specific error handling and messaging.
101: Move this comment to the docstring
107: Have _parse_input raise a specific exception for this scenario, and
catch that; see comment regarding lines 99-107 of transfer_cpio.py
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?
154-160: See comment regarding lines 183-189 of transfer_cpio.py
210/212: Nit: Use "for pkg_install in self._tr_pkg_install:"
(Ditto for line 249/251)
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.
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?
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
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)
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?
499-577: Seems like this duplicates a lot of the code in the prior class
- perhaps some reorganization is in order?
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
