On 07/30/10 07:18 AM, [email protected] wrote:
Hi all,
If you have provided me feedback on the AI Multi-homed feature
code, could you please ensure your desired changes have been affected?
I would ideally like a go/no-go by COB Monday 8/2.
Also, for everyone's information, it was requested that I add a
feature to allow a multi-homed server to masquerade as a single-homed
server. This allows support for older multi-homed unaware images to be
served on a multi-homed server with only some loss of multi-homed
functionality. Lastly, a number of QE found bugs have been addressed
as well.
Full webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed//final_full/
Differential from initial webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed//final_diff/
Original webrev:
http://cr.opensolaris.org/~clayb/webrev_multihomed/
Thank you,
Clay
Hey Clay,
I have already provided feedback to your reply to my comments.
I have taken another look through the code and mostly I only have nits.
Joe
- - -
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/ai_sd.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
I have nothing to add to what Keith and Drew have already commented on.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/installadm-common.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
I have little to add regarding validate_networks/calculate_net_address
beyond the comments from Dave and Ethan.
I am concerned about the complexity of this logic and migrating it
to Python. Perhaps the best thing might be to request enhancement
to exiting network adm tools to have them provide this info.
I will look in to this as it would be valuable for the ISIM
projec.
Question:
---------
Had you investigated using ipadm show-if as we talked about?
I don't see it being done. Was it not viable?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/server.xml
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Thanks!
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-dhcp.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit:
---
These comment doesn't seem to read properly.
104 # value will cause the for loop below to split on
126 # next, check to see if this option is set and
127 # if so, set differently, then save in a
128 # instructions
129 # for the user
Nit/issue:
---------
This is a general comment which could be applied throughout the code.
Perhps this is a style thing but I think having comment describing
blocks of code are more valuable then line by line comments.
I think this could benifit from being compressed from:
126 # next, check to see if this option is set and
127 # if so, set differently, then save in a
128 # instructions
129 # for the user
130 match_macro_and_option $name ${optstr%%=*} \
131 ${optstr#*=}
132 if (( $? == 1 )); then
133 # add a space and option name to
134 # $collision_opts
135 collision_opts+=" ${optstr%%=*}"
136 # add a space and option value to
137 # $collision_opts
138 collision_opts+=" ${optstr#*=}"
139 continue
140 fi
141 optstr=${optstr/\$boot_file/$boot_file}
142 $DHTADM -M -m $name -e $optstr
143 IFS=":"
144 done
145 IFS="$oifs"
146 # print colliding options if any were found
147 [[ -n $collision_opts ]] && print_macro_opt_collision \
148 $name "$collision_opts"
To:
# Generate a space separated list of any matching options. Change
# as needed and save as printable instructions for the user.
match_macro_and_option $name ${optstr%%=*} \
${optstr#*=}
if (( $? == 1 )); then
collision_opts+=" ${optstr%%=*}"
collision_opts+=" ${optstr#*=}"
continue
fi
optstr=${optstr/\$boot_file/$boot_file}
$DHTADM -M -m $name -e $optstr
IFS=":"
done
IFS="$oifs"
if [[ -n $collision_opts ]]; then
print_macro_opt_collision $name "$collision_opts"
Issue:
-----
line 608 down.
Perhaps not worth changing at this point but instead of the
sequence of if/elif a case would be a better choice.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-image.sh
and other .sh files
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit/Style thing.
Why change these to use all the ${CMD} usage?
Why not rely on $PATH?
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips
http://hub.opensolaris.org/bin/view/Community+Group+on/shellstyle
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
