Joe,
Thanks for taking the time to review these changes. I truly appreciate
the extra work. See below.
Thanks,
John
On 12/20/10 01:13 PM, [email protected] wrote:
Looks good John. Just a few minor issues...
I reviewed this list of files:
> Joe/Clay - installadm infrastructure changes
>
> usr/src/cmd/installadm/check-server-setup.sh
> usr/src/cmd/installadm/delete_service.py
> usr/src/cmd/installadm/installadm-common.sh
> usr/src/cmd/installadm/installadm.c
> usr/src/cmd/installadm/installadm.h
> usr/src/cmd/installadm/installadm_common.py
> usr/src/cmd/installadm/installadm_util.c
> usr/src/cmd/installadm/list.py
> usr/src/cmd/installadm/setup-service.sh
> usr/src/cmd/installadm/setup-sparc.sh
Joe
- - -
General Shell script comments:
------------------------------
Follow conventions and guidelines:
----------------------------------
Please see the OpenSolaris Bourne/Korn Shell Coding Conventions:
http://hub.opensolaris.org/bin/view/Community+Group+on/shellstyle
and the Install ksh tips:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips
Use ksh93
http://hub.opensolaris.org/bin/view/Project+ksh93-integration/
Run shcomp on all shell scripts:
--------------------------------
shcomp -n <any shell scripts> /dev/null
Used. I didn't see anything from the code that I changed. I did correct
additional stuff that this pointed out. However, I left one section the
same
as the change caused other issues.
Use shell builtins where possible:
----------------------------------
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#use_true_and_false_ksh93_builtins
Can't access installzone-wiki.central any longer. Not sure where it went.
and consulte the ksh93(1) man page.
To see a list of built-ins issue:
% ksh93 builtin
usr/src/cmd/installadm/check-server-setup.sh 11 lines changed: JJV
usr/src/cmd/installadm/check-server-setup.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
nit:
----
191 # * Check that property all_services/exclude_networks exists
--> 192 # * Check that property all_services/default_port exists
193 # * Check that all networks specified by all_services/networks
match at
Indentation looks off.
Corrected.
nit:
----
256 valid="False"
Use built in true and false vs a string "True" and "False"
e.g.:
$ x=true
$ if $x; then echo yes; fi
yes
Right. But Clay is already using "False" and "True" in the code above and
I did not want to make additional changes elsewhere where he checks the
return.
nit:
----
254 "all_services/default_port = "\
Missing blank between " and \
Inconsistent "<no blank>\
This code is essentially copied from above with the same syntax. I'll
change it above as well.
Please clarify:
---------------
255 "integer: 5555'."
Should this be "5555"? I thought we were going to provide default
value configuration.
This is just a suggestion for the default value. The server.xml has the
default
value in it already. Dave points out that this code and some of the code
preceding
it can be removed because these values should be already defined with
the package.
I can do that now or we can wait for the ISIM changes.
usr/src/cmd/installadm/installadm-common.sh 22 lines changed: JJV
usr/src/cmd/installadm/installadm-common.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
issue:
------
106 return $?
...
928 get_default_http_port
get_default_http_port returned status code is never checked.
Add the following code:
get_default_http_port
if [ $? -ne 0 ] ; then
print "Warning: Unable to determine the service's default port"
print " Using 5555 as the service's port"
HTTP_PORT=5555
fi
usr/src/cmd/installadm/installadm.c 35 lines changed: JJV
usr/src/cmd/installadm/installadm.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
nit:
----
590 service_data_t data;
591 char *pg_name;
592 int port;
593 scfutilhandle_t *hdl;
Line 592 indentation is off.
Corrected.
usr/src/cmd/installadm/installadm_util.c 119 lines changed: JJV
usr/src/cmd/installadm/installadm_util.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Suggestion:
96 char *fmri = "svc:/system/install/server:default";
97 char *propname = "all_services/default_port";
I would suggest #defining these strings in installadm.h
Changed to:
char *fmri = SRV_INSTANCE;
char *propname = PORT_PROP;
and added to installadm.h:
#define SRV_INSTANCE "svc:/system/install/server:default"
#define PORT_PROP "all_services/default_port"
usr/src/cmd/installadm/list.py 172 lines changed: JJV
usr/src/cmd/installadm/list.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/installadm/setup-service.sh 121 lines changed: JJV
usr/src/cmd/installadm/setup-service.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
nit/questio:
62 # check webserver port and that webserver is running on said port
Is that comment accurate stil?
Nope. The correct comment is:
# check to see if the service has an mDNS record registered
I caught that one already and made the change.
usr/src/cmd/installadm/setup-sparc.sh 2 lines changed: JJV
usr/src/cmd/installadm/setup-sparc.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
147 get_default_http_port
Should get_default_http_port return status be checked?
get_default_http_port
if [ $? -ne 0 ] ; then
print "Warning: Unable to determine the service's default port"
print " Using 5555 as the service's port"
HTTP_PORT=5555
fi
