Hi Joe,
Thank you for the comment responses! My changes and comments
below:
Full webrev:
cr.opensolaris.org/~clayb/webrev.multihomed/joe
Differential webrev:
cr.opensolaris.org/~clayb/webrev.multihomed/joe/diff
Thank you,
Clay
On Wed, 14 Jul 2010, [email protected] wrote:
On 07/13/10 06:37 PM, [email protected] wrote:
Hi Joe,
[snip...]
Hey Clay,
Did you get a chance to file a bug for the known issue? If so what's the
bug number? If not can you describe where in the code the issue is?
As this bug should be fixed before push I haven't filed a bug, however, I
did fix it. New webrevs are:
Full:
http://cr.opensolaris.org/~clayb/webrev.multihomed/webrev1
Differential:
http://cr.opensolaris.org/~clayb/webrev.multihomed/webrev1.diff
The bug was in (new line numbers) 463 of installadm-common.sh, 805 of
installadm.c and 575 of setup-dhcp.sh.
Thank you,
Clay
Hey Clay,
All in all I think this looks pretty good. Most of my comments are trivial.
Hope this helps!
Joe
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
General comments:
Issue:
------
Run shcomp on all the scripts:
Thank you for pointing this out. All lines but one were use of `...`
instead of $(...). However, one was for using -ne in [[ ... ]] instead of
(( ... )). They all should be fixed now.
e.g.:
shcomp -n <any shell scripts> /dev/null
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/ai_sd.py
Issue:
------
22 # Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
I think this copyright should be:
# Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights
reserved.
Copyrights should be better now.
Suggestion:
-----------
399 dhcpinfo = Popen(['/sbin/dhcpinfo','BootSrvA'],
400 stderr=PIPE, stdout=PIPE)
build an argslist with the dhcpinfo command e.g.:
argslist = ['/sbin/dhcpinfo','BootSrvA']
dhcpinfo = Popen(argslist, stderr=PIPE, stdout=PIPE)
If thing fail the argslist could be displayed in a message.
I've followed what was previously done at lines 142, 148, 156 to allow
this extension of the code in the future.
Suggestion:
-----------
Would it make sence to isolate the logic between 397 and 410 into
a supporting function?
Yes for testability, if anything, this code needs to be pulled out. I've
not come up with how I want to achieve that yet.
In the future will dhcpinfo be used for other things? If so perhaps a
more generic function.
It certainly could be, but we'll see.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/svc/manifest-locator
Suggestion:
-----------
114 # Check to see if AI_SERVICE_ADDRESS has the $serverIP keyword
115 #
116 if [ -n "$(print "$AI_SERVICE_ADDRESS" | $GREP '$serverIP')" ];
then
117 AI_SERVICE_ADDRESS=$(print $AI_SERVICE_ADDRESS | \
118 $SED "s/\$serverIP/$($DHCPINFO $BOOTSERVER)/")
119 fi
Perhaps you could leverage built in extended regular expression
pattern matching here.
I can, and I've made these changes.
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#Use_built_in_extended_regular_expression_pattern_matching
For external folks, much of the same is at:
http://hub.opensolaris.org/bin/view/Community+Group+on/shellstyle
http://hub.opensolaris.org/bin/view/Project+shell/shellstyle
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/ai-httpd.conf
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/installadm-common.sh
Issue:
------
461 if (( $(valid_networks | $WC -l) != 1 )); then
If it is not 1 then more than 1 is assumed. Wouldn't it be better
to coding for the less than 1 ( == 0 ) case here?
Or less than one is acceptable here too. Primarily, I was wanting to
follow the legacy network behavior if we only have one interface.
Issue:
------
Where SERVICE_ADDRESS_UNKNOWN is printed wouldn't it also be
a good idea to return other than 0?
452 print "$SERVICE_ADDRESS_UNKNOWN"
476 print "$SERVICE_ADDRESS_UNKNOWN"
e.g.:
from:
476 print "$SERVICE_ADDRESS_UNKNOWN"
To:
476 print "$SERVICE_ADDRESS_UNKNOWN"
return 1
It may be, however, I'm not sure what the propagation effects of that
return code may be; as the original author did not pursue that path, I'd
be concerned to do so now.
Question:
---------
493 interfaces=$(ipadm show-addr -p -o ADDR,STATE|\
494 $GREP -v '\\:'|$GREP -v '127.0.0.1' | \
495 $GREP -v '0.0.0.0' | $GREP ':ok$' | $SED
's/:ok$//')
Have you checked with the networking folks to determine if this is
the best way to get the network interface addresses?
I've had an e-mail discussion with Sowmini where she said it was fine to
be using ipadm(1).
I ran that command on my system with an active punchin tunnel. The
results contained the tunnel.
I've since stripped out tunnel interfaces (those with '->')
Suggestion:
-----------
Perhaps you could explore parsing the ipadm output using extended
regular expression pattern matching might be better than piping it
through GREP.
I think readability would suffer parsing this with the shell regular
expressions, as I can envision it today. Certainly Python would be a
cleaner choice for expressing all of the rich data which can be contained
and since most of this data is later manipulated in a manner conducive to
set operations too.
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#Use_built_in_extended_regular_expression_pattern_matching
Suggestion:
-----------
This may be a personal style issue but I would break up function
valid_networks into multiple functions just for readabiltiy/
maintenance.
Certainly, the valid_networks function has become quite rambling!
get_interfaces
doing the ipadm command parsing
get_SMF_networks
doing the stuff under:
509 # get the SMF networks listed for inclusion or exclusion,
get_SMF_exclusion
doing the stuff under:
539 # exclude will be true or false depending on if we are excluding
strip_addr
doing the stuff under:
559 # strip padding zeros off network addresses
Seems like a pretty reasonable breakdown. See what I did and if that helps.
Issue:
------
589 # slurp address into an array
"slurp"? funny but not the most descriptive.
Ah, you're removing the Clay from the code! I'm just kidding, how about
loading address into an array, splitting on dot?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/installadm.c
Issue:
------
488 is_multihomed(void)
This is the similar command to what is performed in
usr/src/cmd/installadm/installadm-common.sh at line 493.
Could this be done in one place and leveraged by the other?
Yes, I've setup is_multihoned() to call installadm-common valid_networks
so that both have the exact same notion of a network.
Issue/Style thing:
------------------
640 if (is_multihomed() != B_FALSE) {
I find the double negative confusing.
if (is_multihomed() == B_TRUE) {
Reads clearer to me. What do you think? Just a style thing perhaps...
Yes, I think just style but I'm equally at home either way, so == B_TRUE
it is.
Question:
---------
729 if (!enable_install_service(handle, service_name)) {
How is enable_install_service performed for the do_create_service case
now that the code around line 729 have been removed?
It's sadly not really obvious thanks to the fragmented joy of installadm.
Like what enable_install_service() does the code in do_create_service()
also calls setup-server register and sets the SMF properties correctly for
a running service.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/installadm.h
Issue:
------
23 * Copyright (c) 2010, Oracle and/or its affiliates. All rights
reserved.
I think this is supposed to have the initial year the source was
licensed.
e.g.:
* Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights
reserved.
Fixed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/server.xml
Issue:
------
23 Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
I think this is supposed to have the initial year the source was
licensed.
e.g.:
* Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights
reserved.
Fixed
Question:
---------
Was there a good reason why you compress these lines? They seemed much
more readable in the initial version.
Yes, just trying xmllint(10, as xmllint --format. Oh well, to see if
anyone can suggest another good automated formatter, if not, yea by-hand
formatting of XML :(
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-dhcp.sh
Suggestion:
-----------
This may be a personal style issue but I would put lines
109->125 into a function just for readibiltiy/mantenance..
I can see how it could be broken up, but as I find the code to be
incredibly ugly to my preference anyways, I don't see breaking it up as a
great change. Python please -- this should be an object operation!
Issue:
------
159 # Print instructions for the user how to manually setup the
160 # network dhcp macro.
That reads oddly to me. I think you need to rephrase that.
Perhaps:
Print the instruction for manually setting up the network dhcp macro.
You want verbs?!? Really? Me programmer good... Find cave and grunt at
screen! I've updated the comment to read:
# Function to show the user how to manually setup the
# network dhcp macro.
Issue:
------
174 if [[ "$sparc" == "true" ]]; then
use true and false ksh93 builtins
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#use_true_and_false_ksh93_builtins
Maybe now might ge a good time to make this change. ?
Sure! That syntax always looks so strange to me, but it looks to work
fine.
Issue:
------
191 # Set up the network dhcp m
I noticed you had updated other files for this webrev to meet
the shell guidelines why not this file?
e.g.:
echo to print and "[ ]" to "[[ ]]" ...
I think I've updated a lot of it now...
Issue:
------
480 for network in $(valid_networks); do
Perhaps valid_networks could be expanded to provide the interface
name using ipadm show-if
valid_networks could parsing the output of ipadm show-addr with
in a loop of all addresses returned by ipadm show-if.
e.g.
for i in $(ipadm show-if -p -o IFNAME); do
print $i;
ipadm show-addr -p -o ADDR,STATE $i/;
<parse ipadm show-addr as is currently done>
done
This way you could also parse out tunnels and return the interface
name with the address.
I think this might simplify some of the logic in function run_setup_dhcp.
Unfortunately, I don't believe this answers the problem of which interface
to use given more than one interface on a network. Here we need to query
the kernel routing table to understand which interface it would use by
default to send packets (thus route(1) or netstat(1) basically).
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-service.sh
Issue:
------
22 # Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
I think this should be:
# Copyright (c) 2009,2010, Oracle and/or its affiliates. All rights reserved.
Fixed
Suggestion:
-----------
116 if (( $(ipadm show-addr -p -o ADDR|\
117 grep -v ':'|grep -v '127.0.0.1' |grep -v '0.0.0.0'|\
118 wc -l) != 1 )); then
119 # for multi-homed AI servers use the "$serverIP" keyword
120 ip="\$serverIP"
Maybe this logic should be in a utility function since it is exercised
in multiple places.
It is more or less, as I've now updated it to use valid_networks and
simply count the response from that.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-sparc.sh
Issue:
------
22 # Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
I think this should be:
# Copyright (c) 2009,2010, Oracle and/or its affiliates. All rights reserved.
Indeed it should be
Issue:
------
195 if [[ $(ipadm show-addr -p -o ADDR|grep -v ':'|grep -v '127.0.0.1' |grep
-v \
196 '0.0.0.0'|wc -l) -ne "1" ]]; then
197 # for multi-homed AI servers use the host's nodename
198 srv_ip="`uname -n`"
As suggested above maybe this logic should be in a utility function
since it is exercised in multiple places.
And this now uses valid_networks() too.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-tftp-links.sh
Looks OK
Yea!
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/slim-install/svc/net-fs-root
Issue:
-----
122 # slurp in hosts files
"slurp"? funny but not the most descriptive.
OK this may initially seem picky but it could be possible that in
the future a non-native-English speaking person may have to maintain
this code. Using slang in the comments might confuse them.
Yes, they'd rightly not be enjoying how the following (from
dictionary.com) apply:
1. to ingest (food or drink) with loud sucking noises: He slurped his
2. to make loud sucking noises while eating or drinking: to slurp when eating
soup.
3. an intake of food or drink with a noisy sucking sound: He finished his
milk in about three slurps.
4. any lapping or splashing sound: the slurp of the waves against the hull.
Thank you for thinking of a non-native speaker as I don't envy such a
hurdle! I've changed "slurp" to "read".
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
