Hi Drew,
Thank you for the read through.
Thank you,
Clay
On Fri, 30 Jul 2010, Drew Fisher wrote:
Clay,
I'm going to focus on the Python changes here.
delete_service.py
=======
483-499: can we un-camelcase the menuLst variable? menu_lst would be
better (and pep8 compliant)
Fixed
ai_sd.py
=======
General Nit: For all of your subprocess calls, you should dump the
exception checking of OSError and ValueError. By definition those are
fatal errors and should propagate up. This has the added benefit of
slimming your code down as you're not constantly re-writing the same
code over and over.
http://docs.python.org/library/subprocess.html#exceptions
36, 38-39: alphabetize your imports
Import alphabetized but still general import; specific import (i.e. from foo
import bar); local imports.
511-519: can you not do try/except/else/finally?
I wasn't even noticing of that nesting. However, the logic here is different;
perhaps there's cleaner to avoid the nesting but I'm not seeing it:
Previously was:
try:
f = open(self.wanboot_conf, "r")
except IOError, err:
yield None, err
else:
try:
yield f, None
finally:
f.close()
I prefer this over the alternative of testing to see if f exists:
f = None
try:
f = open(self.wanboot_conf, "r")
except IOError, err:
yield f, err
else:
yield f, None
finally:
if f:
f.close()
try:
f = open(self.wanboot_conf, "r")
except IOError, err:
yield None, err
else:
yield f, None
finally:
f.close() <-- if we had an exception we don't have an f to close
here and AttributeError!
instead of the nested try/finally?
561-564, 576: wrap these in a try/except so if wanboot.conf ever changes,
you can exit cleanly. I've always found that specifying the index I
want after a split will somehow come back and bite me so I've had to
learn (the hard way) of wrapping ANY calls like this in try/except so
I can handle the errors correctly.
Good point. For 576, I agree; for 561-564, you will always have at least one
list item so split(..)[0] will not fail.
Further, this isn't a possible bug to tickle but is good defensive programming
for future changes. As in order to get to 576, one needs to have a line with at
least '=' which will result in split('=',1) returning at least a two-item list.
580-586: indentation mistake? Why are these not indented under the
class?
Since I reference WANBootInfo.default_wanboot_conf which I can't seem to access
within the class. (Notice not a mistake from line 586 defining it back into the
class. There's got to be a better way?
655-658: be consistent with the commands above. Pass Popen() a list
I see that as inconsistent with lines 142-148; however, I will exactly mimmick
line 148 with the extra variable cmd_args and pulling the split() out of the
Popen() to match the style above.
cmd = ["/sbin/dhcpinfo", "BootSrvA"]
try:
dhcpinfo = subprocess.Popen(cmd, stdout=PIPE, stderr=PIPE)
671: change to check dhcpinfo.returncode. Not everything spits out
to stderr
Thank you. (I like when I can use installadm_common.run_cmd() for not having to
remember all the Popen minutiae.)
676-680: combine to a single return line as you do on line 578
return dhcp_stdout.strip()
Done
======
-Drew
On 07/30/10 05:10, [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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
