Hi John,
Thank you for the questions! Responses below.
Thank you,
Clay
On Tue, 10 Aug 2010, John Fischer wrote:
[snip...]
--------------------------------------------
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.
Actually other-way-around, I believe; the is operator compares if two
objects are the same; or references to the same object.
The == operator compares equality, where as is compares if the objects are
the same. Since Python strings are immutable they usually share the same
object identity (as reported by id(<object>)).
However, if one string is a unicode string that will not hold:
<type 'str'>
type("i386")
<type 'str'>
type(u"i386")
<type 'unicode'>
u"i386"=="i386"
True
u"i386" is "i386"
False
id("i386")
135296512
id(u"i386")
134620608
And in the case of a returned string from a C module, that will not hold;
as, I believe the platform module is a C module.
So the moral of the story, is to use == when you want equality and you are
not doing object magic. Here I just want equality.
--------------------------------------------
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
Wow, I must have replaced nearly 40 concatenations in AI for this code
review...
--------------------------------------------
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:
Since if the property-group doesn't exist, they'll need to add the
properties too; so let's not play fetch a rock but just give them a list
of necessary steps.
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.
They should get that since we don't return.
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.
I've changed it so they should see the quoted string:
'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?
Yes, however, I've abstracted out most of the commands which are used
across scripts; if used in a single script I've ensure the path is fully
qualified at this time. There is still possible improvement but I believe
it to be a small return at this time.
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"`
I've removed any backtick I can find in installadm-common now.
--------------------------------------------
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);
}
True
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) {
Or even installadm_system(CHECK_SETUP_SCRIPT) != 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.
Thank you; that would be a bad omission for usability.
--------------------------------------------
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
Done
--------------------------------------------
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
Ah you don't like sucking up the memory to hold the concatenated hosts
files either? This has been changed
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
I've actually removed the echo on lines 143-144 as wget echos all that
information for us so the redundancy and formatting isn't so great
anyways:
The AI image will be retrieved from /var/ai/install_test_ai_sparc//
directory
Downloading solaris.zlib
--2010-08-11 18:55:00--
http://jumprope:5555/var/ai/install_test_ai_sparc///solaris.zlib
idn_decode failed (9): `System iconv failed'
Resolving jumprope... 172.20.24.78
idn_decode failed (9): `System iconv failed'
Connecting to jumprope|172.20.24.78|:5555... connected.
HTTP request sent, awaiting response... 200 OK
Length: 101922816 (97M) [text/plain]
Saving to: `/tmp/solaris.zlib'
100%[======================================>] 101,922,816 52.8M/s in
1.8s
2010-08-11 18:55:02 (52.8 MB/s) - `/tmp/solaris.zlib' saved
[101922816/101922816]
[...]
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.
Just an oversight from testing likely. Redirecting standard out and then
directing standard error into standard out is the right way (2>&1).
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) ]] ;
No, as we'll create a file even on a partial download -- which is still a
failure.
_______________________________________________
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
