Hi John,
Thank you for the read through of the code. My comments below and
new webrevs below (the webrevs will have Keith's changes as well):
Full webrev:
http://cr.opensolaris.org/~clayb/webrev.multihomed/john
Differential webrev:
http://cr.opensolaris.org/~clayb/webrev.multihomed/john/diff
Thank you,
Clay
On Wed, 14 Jul 2010, John Fischer wrote:
Clay,
Here is my feedback on the original code review.
Thanks,
John
-----------------------------------------
ai_sd.py
-----------------------------------------
Shouldn't the Copyright include 2009 as well as 2010? Or the date of
the
original file creation?
Yup, I messed up my copyrights. They should be happy now.
dhcpinfo is fine for the Popen() but then reassigning and accessing
it gets
a little confusing from a readability/sustainability standpoint:
Fixed, unfortunately I will have to change this around a bit so that
these 10 lines are unit-testable; something I didn't properly do yet.
[snip...]
-----------------------------------------
manifest-locator
-----------------------------------------
When I checked this below I got some weirdness with a $ left in it.
So I am not 100% it works as expected. Not even sure I pulled out
the environment variables correctly to check. I am concerned that the
code is replacing $serverIP with something that might leave in a $ which
might be unexpected. Unless that is expected. 8-)
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
Nope, no $'s should be left in. This is ksh93 and if run in another
shell it might not be happy.
Another way to visualize this:
AI_SERVICE_ADDRESS=$(print $AI_SERVICE_ADDRESS | \
$SED "s/\$serverIP/$($DHCPINFO $BOOTSERVER)/")
Would be:
boot_server=`dhcpinfo BootSrvA`
AI_SERVICE_ADDRESS=`/usr/bin/echo '$serverIP:46501' | /usr/bin/sed
"s/\$serverIP/$boot_servr/"`
-----------------------------------------
installadm-common.sh
-----------------------------------------
The comment should probably be more specific about 'this'. Also is
there a bug
that is being tracked for the Solaris sed(1) not being happy? I
think you just
using the GNU sed's \n feature which Solaris sed(1) does not have.
40 # XXX this needs to go away but Solaris sed(1) isn't happy
41 GNUSED=/usr/gnu/bin/sed
Good questions, the XXX was mainly to get any ideas from code
reviewers on how better to do this; I think I've found a solution
using ksh pattern matching syntax.
Either way, for interested parties, the bug is:
CR1218983 - *sed*: sed does not provide for line splitting
I hesitate using GNU sed (or the POSIX sed's as they aren't as much a
core part of the OS as /usr/bin/sed). And yes, GNU sed was the this I
was referring to, in awesome ambiguity.
Why assume multihomed if no interfaces are up? Mostly for my FYI.
459 # see if the machine has more than one interface up/no
interfaces up
460 # or if the hostname is the keyword "$serverIP" we are
multihomed
461 if [[ $(valid_networks | $WC -l) -ne "1" || \
462 "$svc_address_hostname" == '$serverIP' ]]; then
463 # for multi-homed AI servers use the "$serverIP"
keyword
464 srv_address_ip="$serverIP"
465 else
466 srv_address_ip=`get_host_ip "$srv_address_hostname"`
467 fi
No problem, this is due to if no interfaces are up we don't have IPs
to populate with anyways but everything for multihomed can work, so we
may as well support it. Similarly, this makes it easier for an
admin/salesman/developer/etc. who wants a laptop that can act as a
general AI server despite what network it's plugged into. (Though they
would still need to manually configure DHCP for the particular network
specifics.)
Does below really get us the interfaces? Or a list of IP addresses?
Isn't
the interface ADDROBJ? Perhaps a different name for the variable.
Also should tunnels be removed from the list? $GREP -v '->' If this
is not
pulled out then 'arithmetic syntax error' will be spewed from
calculate_net_addrs.
Thank you for pointing out tunnels. I've now updated the grep for this
too.
Why not use egrep for the exclusions instead? $EGREP -v
'\\:|127.0.0.1|0.0.0.0'
Oversight, replaced with egrep(1):
$EGREP -v '\\:|^127.0.0.1|^0.0.0.0|[0-9]->[0-9]|' | $GREP ':ok$'
If you do this don't forget to include the tunnels issue as well.
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$//')
I've included skipping tunnel interfaces.
Nit - beggining should be beginning.
499 # check beggining address as that is what the
DHCP server will
Sigh, I wish aspell(1) was still in /contrib...
Nit - otherwis should be otherwise
552 # strip the trailing space off match_nets
as otherwis
Thank you
-----------------------------------------
installadm.c
-----------------------------------------
I don't really care for the comments interspersed with the command
string in the
is_multihomed() function. To me it makes the function call harder to
read.
you also might need to strip out tunnels (->)?
491 (void) snprintf(cmd, sizeof (cmd),
492 /* use the shell to see if more than one interface
has link */
493 "/usr/bin/[ `/usr/sbin/ipadm show-addr -p -o
ADDR,STATE| "
494 /* strip out loopback, unconfigured DHCP, IPv6
interfaces and */
495 /* those that are up -- have link, etc. */
496 "/usr/bin/egrep -v '\\:|127.0.0.1|0.0.0.0'|
/usr/bin/grep ':ok$'|"
497 /* use wc(1) to count interfaces */
498 "/usr/bin/wc -l` -eq '1' ]");
This has been replaced to a much cleaner call to valid_networks out of
installadm-common, thus ensuring that it gets the same notion of an
interface as everything else:
/* use the shell to see if system is multihomed by calling */
/* valid_networks() from installadm-common and using wc(1) to
count */
(void) snprintf(cmd, sizeof (cmd),
"/usr/bin/[ `%s -c 'source %s; valid_networks' | %s -l` -eq
'1' ]",
KSH93, INSTALLADM-COMMON_SCRIPT, WC);
multiple calls to is_multihomed() seems a little expensive. Calling
this once
and setting a (possibly global) variable would be more efficient.
640 if (is_multihomed() != B_FALSE) {
662 if (is_multihomed() == B_TRUE) {
871 is_multihomed()?"\\$serverIP":server_ip,
882 is_multihomed()?"\\$serverIP":server_ip,
dhcp_macro, dhcpbfile);
It doesn't look like it should be much of a performance problem, I
don't think:
ptime /usr/bin/[ `ksh -c "source
/usr/lib/installadm/installadm-common; valid_networks | wc -l` -ne 1 ]
real 0.002435729
user 0.000391311
sys 0.001702111
-----------------------------------------
installadm.h
-----------------------------------------
Shouldn't the Copyright include 2009 as well as 2010? Or the original
creation
of the file and 2010?
Yup, 2008 (Tue Oct 07 14:28:42 2008 -0600) actually.
-----------------------------------------
server.xml
-----------------------------------------
Same copyright issue.
Similarly fixed....
For me the formating of the file was easier to read before.
The original looks like:
43 <dependency
44 name='dns-multicast'
45 type='service'
46 grouping='optional_all'
47 restart_on='restart'>
48 <service_fmri value='svc:/network/dns/multicast:default' />
49 </dependency>
The new one looks like:
35 <dependency name="dns-multicast" type="service"
grouping="optional_all" restart_on="restart">
36 <service_fmri value="svc:/network/dns/multicast:default"/>
37 </dependency>
The rest of the file is similarly condensed.
This is thanks to using xmllint --format. I'd like to find a pragmatic
way to format the XML, it seems by-hand is a bit nebulous. Do you have
any suggestions on what you'd use?
Also, we have talked about how I had inadvertantly setup the
all_services property_group as a service property and not an instance
property group. I have corrected that and it is now setup as a
instance property group here.
-----------------------------------------
setup-dhcp.sh
-----------------------------------------
I know this seems insignificant but why are you changing all the
function
definitions from 'name()' to 'function name'?
From the OpenSolaris shell-style guide[1] it's for performance:
Use ksh-style function
It is highly recommended to use ksh style functions (function foo {
... }) instead of Bourne-style functions (foo() { ... }) if possible
(and local variables instead of spaming the global namespace).
[1]: OpenSolaris shell style guide:
http://hub.opensolaris.org/bin/view/Project+shell/shellstyle#use_ksh_style_function_syntax
Is there any significance to you using [[ ]] instead of [ ] for
conditionals?
The majority of the code only uses [ ] for conditionals.
Similarly, for performance and better features. See the style guide
but look for: Use "[[ expr ]]" instead of "[ expr ]"
print_dhcp_macro_info seem more descriptive then
print_service_macro_info.
Ah, I see you separated out the message(s) into 2 functions. In that
case
lines 150-155 are duplicate output to lines 181-186. Would the customer
ever see both sets of messages? If so then perhaps reworking the
messages
some more is in order. Don't want to nag the customer. :-)
If you are running the Solaris DHCP server, use the following
....
If you are running the Solaris DHCP server, use the following
....
Yes, there is some nagging going on. It's possible to get one set or
the other. What would you envision as a better format? (Certainly,
there should be one!)
Currently:
------------------------------------------------------------------------
[from print_service_macro():]
Setting up the target image at /var/ai/install_test_ai_sparc ...
If not already configured, please create a DHCP macro
named 10.10.1.25 with:
Boot server IP (BootSrvA) : 10.10.1.25
If you are running the Solaris DHCP Server, use the following
command to add the DHCP macro, 10.10.1.0:
/usr/sbin/dhtadm -g -A -m 10.10.1.0 -d :BootSrvA=10.10.1.25:
Note: Be sure to assign client IP address(es) if needed
(e.g., if running the Solaris DHCP Server, run pntadm(1M)).
Setting up the target image at /var/ai/install_test_ai_sparc ...
If not already configured, please create a DHCP macro
named 172.20.24.0 with:
Boot server IP (BootSrvA) : 172.20.24.78
If you are running the Solaris DHCP Server, use the following
command to add the DHCP macro, 172.20.24.0:
/usr/sbin/dhtadm -g -A -m 172.20.24.0 -d :BootSrvA=172.20.24.78:
Note: Be sure to assign client IP address(es) if needed
(e.g., if running the Solaris DHCP Server, run pntadm(1M)).
Setting up the target image at /var/ai/install_test_ai_sparc ...
If not already configured, please create a DHCP macro
named 192.168.1.0 with:
Boot server IP (BootSrvA) : 192.168.1.1
If you are running the Solaris DHCP Server, use the following
command to add the DHCP macro, 192.168.1.0:
/usr/sbin/dhtadm -g -A -m 192.168.1.0 -d :BootSrvA=192.168.1.1:
Note: Be sure to assign client IP address(es) if needed
(e.g., if running the Solaris DHCP Server, run pntadm(1M)).
Detected that DHCP is not set up on this server.
If not already configured, please create a DHCP macro
named dhcp_macro_install_test_ai_x86 with:
Boot file (BootFile) : install_test_ai_x86
If you are running the Solaris DHCP Server, use the following
command to add the DHCP macro, dhcp_macro_install_test_ai_x86:
/usr/sbin/dhtadm -g -A -m dhcp_macro_install_test_ai_x86 -d
:BootFile=install
Note: Be sure to assign client IP address(es) if needed
(e.g., if running the Solaris DHCP Server, run pntadm(1M)).
-------------------------------------------------------------------
Perhaps better:
-------------------------------------------------------------------
Setting up the target image at /var/ai/install_test_ai_sparc ...
If not already configured, please use dhtadm(1) to create or updated
the following DHCP macros:
Macro name Options
10.10.1.0 Update with: BootSrvA=10.10.1.25
172.20.24.0 Update with: BootSrvA=172.20.24.78
192.168.1.0 Update with: BootSrvA=192.168.1.1
dhcp_macro_install_test_ai_x86 Create with:
BootFile=install_test_ai_x86
Though this would not provide as detailed of information as we
previously have, I don't think we do want to be as detailed for every
macro when multihomed systems can require a plethora of macro changes?
Nit - preferrably should be preferably
79 # overwrite is true, unless otherwise set (preferrably to
append)
Thank you
Nit - escpaed should be escaped
496 # escpaed :'s (IPv6); next look for lines which
Thank you again
Nit - settting should be setting
562 # an odd error indicates a failure in settting up network
macros)
Aw, just when I thought I could slip three t's into a word and no one
would notice. Thank you for catching the spelling stupidity here.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
