Hi Clay,
I used this webrev location for my review:
http://cr.opensolaris.org/~clayb/webrev.multihomed/john
I tried not to repeat other's comments...I reviewed all files, only
noted those for which I had comments.
thanks,
sarah
******
ai_sd.py:
404 # dhcpinfo(1) should return a string akin to '192.168.5.1\n'
405 svc_address = dhcp_stdout.strip()
406
407 # check if we got an error or if we got no useful response
408 if dhcp_stderr or not svc_address:
409 AISD_LOG.post(AILog.AI_DBGLVL_ERR,
410 "Couldn't find BootSrvA record in DHCP; error
%s",
411 dhcp_stderr)
412 return 2
Wouldn't it be better to check for dhcp_stderr first, before trying to
strip out the address?
412 return 2
What does a '2' return mean to the caller?
installadm-common.sh:
97 #<get all addresses> |
498 #<remove IPv6> |<remove lo0> |
499 #<remove unconfigured>
Why do you remove ipv6 addresses here?
# exclude will be true or false depending on if we are excluding
557 # the networks configured in all_services/networks
558 exclude=$($SVCPROP -cp all_services/exclude_networks
$SMF_INST_SERVER)
559
Why is the exclude list different than the all_services/networks list?
Not saying it's wrong, just wondering why it is this way and why we have
to traverse the networks list twice to get the final list.
584 for octet in $net; do
585 # XXX I must be missing a better way to do s/^0*//
586 octet=${octet#0}
587 stripped_net="${stripped_net}${octet#0}."
588 done
Can't you use the ${parameter//pattern/string} capabilities of the
scripting to do this substitution?
The XXX comment should be removed if not better way exists.
637 if (is_multihomed() != B_FALSE) {
I would think it would read better as "if is_multihomed == B_TRUE) {...
setup-dhcp.sh:
changes to add_macro for updating as opposed to overwriting.. Is there a
bug related to this change? It wasn't obvious to me in the bugs listed.
Just wondering which piece covers this change.
112 for option in $value; do
113 # ignore blank matches (the leading colon
114 # in the macro string)
115 if [[ -z "$option" ]]; then
116 continue
117 fi
118 IFS="$OIFS"
119 option=${option/\$boot_file/$boot_file}
120 $DHTADM -M -m $name -e $option
121 IFS=":"
Is it possible that an option specified would be invalid? If so, do we
care? Or will it fail in another place?
setup-service.sh:
115 # see if the machine has more than one interface up or no
interfaces up
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"
This code block is repeated in installadm-common, can we create one
function and use it?
addr=`$PNTADM -P ${net} | /usr/bin/nawk '{ print $3 }' \
455 2>/dev/null | $GREP "^${ip}\$"`
This code is repeated serveral times in setup-dhcp.sh. Can we create a
function and just call it? Might be easier to maintain. Also,
/usr/bin/nawk should be globally defined as NAWK and used this way.
interface=$(route -n get \
485 ${network%*.[0-9]*}.$((${network##*.}+1)) | $GREP \
486 '^[^a-zA-Z0-9]*interface: ' | $SED \
487 's/^[^a-zA-Z0-9]*interface: //')
So, I am wondering if we are trying to get the interface the ip address
why don't you just use ipadm show-addr, and grep through the output to
find the network address you are looking for then get the interface?
Then you can proceed to the code starting at line 498. I am concerned
about using the routing tables to get this data.
setup-sparch.sh:
39 WANBOOTCGI_PATH="/usr/lib/inet/wanboot/"
40 WANBOOTCGI="wanboot-cgi"
Why did you separate these into two variables? It doesn't look like we
use them independently.
thanks,
sarah
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
