On 07/13/10 04:04 AM, [email protected] wrote:
Hi all,
Could Joe and John please look this over and anyone else who might
be interested? This is the wad of changes for introducing multihomed
support to AI. Please get me your impressions by Monday 7/19.
There is a known bug right now that create-client for X86 doesn't use
the $serverIP keyword as it should yet, so please be cautious of that
in your review.
The code review is posted at:
http://cr.opensolaris.org/~clayb/webrev.multihomed/
Thank you,
Clay
Hi Clay,
I have a few comments. Still working on my shell scripting "best
practices" so I likely missed a few things in those files.
ai_sd.py:
Copyright should be 2008, 2010
As mentioned offline, this code could be unit tested pretty easily by
re-factoring the changes into a separate function.
402: For readability, this line might be better off as:
dhcpout, dhcperrr = dhcpinfo.communicate() # or something similar
Also, I'm not sure if dhcpinfo currently could end up doing this or not,
but what happens if both stdout and stderr are empty? It looks like
"svc_address" should be checked to make sure it's not empty after line 409.
manifest-locator:
Is there a reason "print" is used on line 116, but "echo" is used on 154
& 338?
installadm-common.sh:
Copyright should be 2008, 2010.
567: This seems to work:
expr match "$octet" '0*\([^0]*\)'
777:
I think the $( <command> ) notation is preferred for legibility.
That also appears to be a bunch of either spaces or a tab - I know it's
unrelated but would it be possible to switch to a "\t" if that's the
case (and if it actually works that way)?
840: $( ... )
879, 888: "\t"
installadm.c:
Copyright: 2008, 2010
server.xml:
The reformatting of this file makes it much more difficult to read.
Would it be possible to revert?
setup-dhcp.sh:
Copyright 2008, 2010
593-597: With the change, the if/else seems redundant - just "exit $status".
setup-tftp-links.sh:
Copyright 2008, 2010
86-87: Could this be one line? (Should wc be defined at the top?)
number_of_nets="$(valid_networks | wc -l)"
setup-sparc.sh:
Copyright 2009, 2010
setup-service.sh:
Copyright 2008, 2010
115-120: I'm noticing this code is repeated in a number of places - is
there any way to "common-ize" it? Provide a small utility script that
could be referenced from multiple places or some such thing?
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
