Hi Keith,
My apologies, however, working through John's code-review I found
that somehow things got confused and everything is appearing as a SPARC
image. I've pulled down the updated webrev and will be reposing when I
figure out what happened.
Thank you,
Clay
On Thu, 15 Jul 2010, [email protected] wrote:
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
