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:
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.
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.
Suggestion:
-----------
Would it make sence to isolate the logic between 397 and 410 into
a supporting function?
In the future will dhcpinfo be used for other things? If so perhaps a
more generic function.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#Use_built_in_extended_regular_expression_pattern_matching
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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?
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
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 ran that command on my system with an active punchin tunnel. The
results contained the tunnel.
Suggestion:
-----------
Perhaps you could explore parsing the ipadm output using extended
regular expression pattern matching might be better than piping it
through GREP.
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.
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
Issue:
------
589 # slurp address into an array
"slurp"? funny but not the most descriptive.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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?
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...
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?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
Question:
---------
Was there a good reason why you compress these lines? They seemed much
more readable in the initial version.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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..
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.
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. ?
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 "[[ ]]" ...
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.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
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.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
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.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-tftp-links.sh
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
