Hi Keith,
Thank you for the code review! My responses in-line. New webrevs
are:
Full:
http://cr.opensolaris.org/~clayb/webrev.multihomed/keith
Differential:
http://cr.opensolaris.org/~clayb/webrev.multihomed/keith/diff
Thank you,
Clay
On Wed, 14 Jul 2010, Keith Mitchell wrote:
On 07/13/10 04:04 AM, [email protected] wrote:
[snip...]
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.
No problem, it was certainly rusty in my head. Though ravaged a bit by the
XWiki move, I think the best instructions I've seen are:
http://hub.opensolaris.org/bin/view/Project+shell/shellstyle
ai_sd.py:
Copyright should be 2008, 2010
Yes, I clearly forget my copyright style. These should be fixed now.
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
Not a bad idea, implemented
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.
Good point, I've added a check in for a non-useful response
manifest-locator:
Is there a reason "print" is used on line 116, but "echo" is used on 154 &
338?
Just following what was used before. As print is a ksh built-in and
echo(1) is not (but echo is built-in too but ambiguous), I've changed all
the echo's to print's.
installadm-common.sh:
Copyright should be 2008, 2010.
Fixed
567: This seems to work:
expr match "$octet" '0*\([^0]*\)'
My main concern is the for loop having to split on octets; I was trying
to nail down a nice shell built-in way to use a reg. ex. I think the
pattern matching used here is more compact.
I think 'expr match' is using expr(1) and forking a subprocess gets
expensive after a while.
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)?
Yes, \t is a lot more readable. However, I've found it a bit strange
as look at this odd behavior in /bin/grep:
[/tmp/test]:
foo
^Ibar
^Ibaz
[matches only a space or tab beginning the line (but not space then a tab(?))]
cl...@opensolaris:/tmp/C$ grep "^[ \t]" /tmp/test
foo
baz
[matches spaces or tabs beginning a line]
cl...@opensolaris:/tmp/C$ grep "^[ \t]*" /tmp/test
foo
bar
baz
[matches spaces or tabs beginning a line]
cl...@opensolaris:/tmp/C$ grep "^[ ^I]" /tmp/test
foo
bar
baz
[matches spaces beginning a line]
cl...@opensolaris:/tmp/C$ grep "^[ ]" /tmp/test
foo
baz
This makes me uneasy about using the \t notation (especially being that
I'm pretty used to tabs running around a shell-script).
840: $( ... )
Fixed
879, 888: "\t"
Again, as I don't know the ramifications of changing to the \t syntax,
I'm going to defer for now.
installadm.c:
Copyright: 2008, 2010
Fixed
server.xml:
The reformatting of this file makes it much more difficult to read. Would it
be possible to revert?
I simply used xmllint --format on the file to use the standardized
formatting. I'd prefer to use a standard tool for formatting our XML
rather than doing it by hand. Do you know of perhaps another tool I could
employ or if different xmllint options would be preferable?
setup-dhcp.sh:
Copyright 2008, 2010
593-597: With the change, the if/else seems redundant - just "exit $status".
I agree! Fixed. I also took out the goofiness in the second webrev for
lines 587-589 which were redundant.
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)"
I really hate how many globals are used in this file already. See webrev1
for how this was one-lined as pretty much just as you envisioned.
setup-sparc.sh:
Copyright 2009, 2010
Fixed
setup-service.sh:
Copyright 2008, 2010
Fixed
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?
Actually, not only is the ipadm(1) call repeated a number of places but
what I want in most places is the number of distinct networks -- not just
the number of interfaces[1]. As such, I should be using valid_networks()
instead. I've changed valid networks to cache its answer and updated the
couple of locations which used ipadm(1) to use valid_networks when
applicable.
[1]: Why we don't concern ourselves if all interfaces are on the same
network is we can publish any working IP address for the system and
clients should be able to route there being on that network segment. This
assumption does not hold in all multihomed, multiple-network environment.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
