Clay,
You've been busy. Lots of good work here.
I know that it is pretty late in the review cycle but here are a few more
comments.
Thanks,
John
--------------------------------------------
usr/src/cmd/auto-install/ai_sd.py
--------------------------------------------
For my education. Still learning Python. Why does:
395 if svc_address == "\$serverIP" and platform.processor() == "i386":
work and:
if svc_address is "\$serverIP" and platform.processor() is "i386":
not work? It has something to do with the svc_address check.
--------------------------------------------
usr/src/cmd/auto-install/svc/manifest-locator
--------------------------------------------
Replace the concatenations 'Couldn't':
129 if [ ! -x $AISD_ENGINE ] ; then
130 print "Couldn't find Auto Installer Service
Discovery Engine" |
131 $TEE_LOGTOCONSOLE
132 return 1
175 if [ ! -x $AISC_ENGINE ] ; then
176 print "Couldn't find Auto Installer Service
Choosing Engine" |
177 $TEE_LOGTOCONSOLE
178 return 1
179 fi
186 if [ $? -ne 0 ] ; then
187 print "Couldn't obtain valid configuration manifest" |
188 $TEE_LOGTOCONSOLE
189 return 1
190 fi
--------------------------------------------
usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml
--------------------------------------------
Looks fine.
--------------------------------------------
usr/src/cmd/installadm/ai-httpd.conf
--------------------------------------------
Looks fine. Does this need a CDDL header?
--------------------------------------------
usr/src/cmd/installadm/check-server-setup.sh
--------------------------------------------
Why not simply return at this point:
152 if ! $SVCPROP -cp all_services $SMF_INST_SERVER \
153 > /dev/null 2>&1; then
154 print_err "Please add the property group" \
155 "all_services:\n" \
156 "svccfg -s $SMF_INST_SERVER addpg" \
157 "all_services application."
158 valid="False"
Perhaps adding a comment that they also need to add
the exclude_networks property before returning.
Also the command should probably be in quotes as this
almost looks like they need to do the following:
svccfg -s svc:/system/install/server:default addpg all_services
application.
Notice the period at the end.
The same quote comment about the all_service/exclude_networks command:
165 print_err "Please add the property" \
166 "all_services/exclude_networks:\n" \
167 "svccfg -s $SMF_INST_SERVER setprop" \
168 "all_services/exclude_networks = "\
169 "boolean: false."
svccfg -s svc:/system/install/server:default setprop \
all_services/exclude_networks = boolean: false.
The same quote comment for the all_service/networks command:
181 print_err "Please add the property" \
182 "all_services/networks:\n" \
183 "svccfg -s $SMF_INST_SERVER setprop" \
184 "all_services/networks = "\
185 "net_address_v4: 0.0.0.0/0."
svccfg -s svc:/system/install/server:default setprop \
all_services/networks = net_address_v4: 0.0.0.0/0.
--------------------------------------------
usr/src/cmd/installadm/create_client.py
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/delete_service.py
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/installadm-common.sh
--------------------------------------------
Seems odd to be using $GREP, $SVCPROP, ... and then also use
/usr/sbin/route,
/usr/bin/getent, ... Doesn't the style guide talk about this issue?
Should this:
858 title=`print title ${grub_title_string} | $SED -e 's/ //g'`
use the $() notation instead?
Same comment for:
968 line=`$GREP "^${IMAGE_BOOTDIR}[ ]" /etc/vfstab`
971 mountpt=`print $line | $CUT -d ' ' -f3`
1047 device=`print ${line} | ${AWK} '{print $1}'`
1241 if_network=`calculate_net_addr
${t_ipaddr}/$bits`
1248 ip_network=`calculate_net_addr ${ipaddr}/$bits`
1286 print `find_network_attr $1 "network"`
1303 print `find_network_attr $1 "netmask"`
1320 print `find_network_attr $1 "netIPaddr"`
--------------------------------------------
usr/src/cmd/installadm/installadm.c
--------------------------------------------
Could rewrite code block:
636 if (dhcp_setup_needed) {
637 if (is_multihomed() == B_TRUE) {
638 (void) fprintf(stderr,
MSG_MULTIHOMED_DHCP_DENY);
639 return (INSTALLADM_FAILURE);
640 }
641 }
As:
if (dhcp_setup_needed && is_multihomed() == B_TRUE) {
(void) fprintf(stderr, MSG_MULTIHOMED_DHCP_DENY);
return (INSTALLADM_FAILURE);
}
Could rewrite:
1139 char cmd[MAXPATHLEN];
1181 (void) snprintf(cmd, sizeof (cmd), "%s",
1182 CHECK_SETUP_SCRIPT);
1183 if (installadm_system(cmd) != 0) {
As:
char *cmd = CHECK_SETUP_SCRIPT;
if (installadm_system(cmd) != 0) {
--------------------------------------------
usr/src/cmd/installadm/installadm.h
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/server.xml
--------------------------------------------
You should probably add documentation to the template section, right
after this section:
102 <documentation>
103 <manpage title='installadm' section='1M' />
104 </documentation>
Perhaps something like:
<pg_pattern name="all_services" type="application">
<description>
<loctext xml:lang='C'>
Properties that control the configuration
of the services
</loctext>
</description>
<prop_pattern name="exclude_networks" type="boolean">
<description>
<loctext xml:lang='C'>
Exclude/Include the networks listed in
networks property
</loctext>
</description>
</prop_pattern>
<prop_pattern name="networks" type="net_address_v4">
<description>
<loctext xml:lang='C'>
List of networks.
</loctext>
</description>
</prop_pattern>
</pg_pattern>
See smf_template(5) for details.
--------------------------------------------
usr/src/cmd/installadm/setup-dhcp.sh
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/setup-image.sh
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/setup-service.sh
--------------------------------------------
Looks good.
--------------------------------------------
usr/src/cmd/installadm/setup-sparc.sh
--------------------------------------------
The code would be more readable/understandable if these lines were
a single line:
197 # we are single-homed
198 else
like:
else # we are single-homed
--------------------------------------------
usr/src/cmd/installadm/setup-tftp-links.sh
--------------------------------------------
Looks good.
--------------------------------------------
net-fs-root
--------------------------------------------
Since hosts is not used again:
123 # concatentate host files from system and wanbootfs
124 if [[ -f ${NETBOOT}/etc/hosts ]]; then
125 # read in hosts files
126 hosts=$(cat /etc/inet/hosts ${NETBOOT}/etc/hosts)
127 # print hosts files out to /etc/inet/hosts
128 print "$hosts" > /etc/inet/hosts
129 fi
could be:
if [[ -f ${NETBOOT}/etc/hosts ]]; then
cat ${NETBOOT}/etc/hosts >> /etc/inet/hosts
fi
This seems like it should be moved lower.
143 echo "The AI image will be retrieved from $ai_image_dir" \
144 "directory" > /dev/msglog
It would be good to move this output down to line 160 after we are
sure that we know all the MEDIA information. Additionally it would
be good to include the location (i.e., server) that this is being
pulled from. So perhaps an echo like:
echo "The AI image found on $ai_server will be retrieved from the" \
$ai_image_dir "directory"
Even if you do not move the echo statement the following else clause that
handles the mounted media (PRTCONF) should at least have a similar echo
statement. Probably somewhere around line 188.
162 else
163 # Get the install media data to use in http url
164 MEDIA=$($PRTCONF -v /devices | \
165 $SED -n '/install_media/{;n;p;}' | \
166 $CUT -f 2 -d \')
167
168 # if the AI server is the $serverIP keyword then the boot
server
169 # needs to be substituted
170 if [ -n $(print $MEDIA | $GREP '$serverIP') ]; then
171 if [ -z $($DHCPINFO $BOOTSERVER) ] ; then
Mainly for my onw education....
228 while (( tries < TRIES )) && ! $cmd > /dev/msglog 2>
/dev/msglog; do
Any reason why you are using '2>/dev/msglog' and not '2>&1'? I ask
because I
see that you use the 2>&1 in setup-dhcp.sh.
Wondering if this should be changed:
234 # if we used all the tries, then we failed
235 if (( tries >= TRIES )); then
would it be better to check for the existence of the file instead?
if [[ ! -s ./$(basename $file) ]] ;
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
