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?
dhcpinfo is fine for the Popen() but then reassigning and accessing it
gets
a little confusing from a readability/sustainability standpoint:
399 dhcpinfo = Popen(['/sbin/dhcpinfo','BootSrvA'],
400 stderr=PIPE, stdout=PIPE)
401 # get (stdout, stderr) tupple
402 dhcpinfo = dhcpinfo.communicate()
403 if dhcpinfo[1]:
404 AISD_LOG.post(AILog.AI_DBGLVL_ERR,
405 "Couldn't find BootSrvA record in DHCP;
error %s",
406 dhcpinfo[1])
407 return 2
408 # dhcpinfo(1) will return a string akin to '192.168.5.1\n'
409 svc_address = dhcpinfo[0].strip()
Perhaps this could be changed to be something like:
dhcpinfo = Popen(['/sbin/dhcpinfo','BootSrvA'],
stderr=PIPE, stdout=PIPE)
(dhcpout, dhcperr) = dhcpinfo.communicate()
if dhcperr:
AISD_LOG.post(AILog.AI_DBGLVL_ERR,
"Couldn't find BootSrvA record in DHCP; error %s",
dhcperr)
return 2
# dhcpout will return a string akin to '192.168.5.1\n'
svc_address = dhcpout.strip()
-----------------------------------------
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
-----------------------------------------
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
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
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.
Why not use egrep for the exclusions instead? $EGREP -v
'\\:|127.0.0.1|0.0.0.0'
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$//')
Nit - beggining should be beginning.
499 # check beggining address as that is what the
DHCP server will
Nit - otherwis should be otherwise
552 # strip the trailing space off match_nets
as otherwis
-----------------------------------------
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' ]");
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);
-----------------------------------------
installadm.h
-----------------------------------------
Shouldn't the Copyright include 2009 as well as 2010? Or the original
creation
of the file and 2010?
-----------------------------------------
server.xml
-----------------------------------------
Same copyright issue.
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.
-----------------------------------------
setup-dhcp.sh
-----------------------------------------
I know this seems insignificant but why are you changing all the function
definitions from 'name()' to 'function name'?
Is there any significance to you using [[ ]] instead of [ ] for
conditionals?
The majority of the code only uses [ ] for conditionals.
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
....
Nit - preferrably should be preferably
79 # overwrite is true, unless otherwise set (preferrably to
append)
Nit - escpaed should be escaped
496 # escpaed :'s (IPv6); next look for lines which
Nit - settting should be setting
562 # an odd error indicates a failure in settting up network
macros)
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
