On 07/30/10 04:18 AM, [email protected] wrote:
Hi all,
If you have provided me feedback on the AI Multi-homed feature
code, could you please ensure your desired changes have been affected?
I would ideally like a go/no-go by COB Monday 8/2.
Also, for everyone's information, it was requested that I add a
feature to allow a multi-homed server to masquerade as a single-homed
server. This allows support for older multi-homed unaware images to be
served on a multi-homed server with only some loss of multi-homed
functionality. Lastly, a number of QE found bugs have been addressed
as well.
Full webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed//final_full/
Differential from initial webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed//final_diff/
Original webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed/
Thank you,
Clay
Hi Clay,
All my prior comments on shell scripts have been addressed. All comments
below are from ai_sd.py, which seems to have gotten rather complicated
suddenly.
120,129: Should this be... "method"? (Why is method listed at all? It's
right above the line, and any pydoc parser should have a reference to
the method name)
405, 418: platform.processor() should provide a more parseable result,
particularly for sparc.
469-509: Would probably be best to move this doctest to a separate
PyUnit test. It's a bit verbose to read which significantly reduces its
value as a test-that-also-documents-usage - ditto for the __getattr__
function's doctest. Given what's being tested - the opening of a file -
there may not really be a need to test this. I trust that the
implementation of "open" in Python works.
558-559: Between these lines, and the previous function, that's a lot of
effort to: open a file, catch the error, tell the caller there was an
error, then have the caller re-raise an error. Let Python's builtin
exceptions do what they're made for, and just open the file directly
(using a "with" block to automatically close it):
with open(a_file) as the_file:
my_data = the_file.readlines()
# process my_data, etc.
#(File closes automatically at end of 'with' block, much like a finally
clause)
563-564, 576: A __getattr__ function that reads a file twice is very
expensive for attribute access. I appreciate the magic of Python used
here, but given the limited usage of this class, it seems a bit
expensive. A simple method (rather than a class), that reads the file
and returns a dictionary of the key/value pairs found in the file would
probably be far more efficient and valuable. If it makes it more natural
to store that data in a class that has a "read from file" function, then
that'd be ok too.
If a full fledged class is required, perhaps the data could be cached in
__dict__, and only re-read the file if the file has been updated since
the last read. (os.stat(filename).st_mtime)
Additionally, regardless of implementation, use str.partition rather
than split. It's a far 'safer' function for cases of the form
"key=value", as it will always return a tuple of length 3. Also, is it
truly technically invalid to have multiple entries of the same name in a
wanboot.conf file? bootconfchk(1M) seems to accept it, so for
consistency, if it's valid, we should as well.
580: If this stays as a class, please wrap this into the class
definition (setting file=None, and then setting wanboot_conf =
WANBootInfo.default_wanboot_conf when file is None should do it). Also,
use "file_" rather than overriding the builtin "file" (similarly, for
readability above when creating a file, use "file_" - or a more
descriptive name - rather than "f").
590/600: Perhaps just create a function?
655-680: Expanding on Drew's comments on use of Popen - if it raises an
OSError, that means a required program (dhcpinfo) is missing; if it's
missing than either there's something corrupt on the image/system, or
there's an IPS dependency error. Both are situations that the ai_sd
can't do anything about; so the best course of action is to propagate up
(at the highest level, e.g in the if __name__ == __main__ block, all
exceptions should be logged and printed to the user with as much
information as possible to assist in evaluating the root cause). Masking
the OSError is a bad idea - let it show through, as that will be easier
to diagnose if the situation ever arises, and has far richer data than
the DHCPError will.
For ValueError, it should only arise in one of two cases: programming
error, or passing in a dynamic (e.g. user-selected) value. In the former
case (which is the case for the usage scenarios in this file), again,
masking with a DHCPError would make it hard to debug; in the latter, it
would be acceptable to catch, but only by the portion of the program "in
control of" the dynamic value.
Overall, the original implementation was much cleaner; really, it just
needed to check the retcode of the Popen object and assert that stdout
returned a usable value (and raise an exception as needed).
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
