On 12/13/10 07:46 PM, John Fischer wrote:
All,
Ethan found an issue with aimdns. Thanks Ethan!!
I have updated the webrev with the changes. The new webrev is at:
http://cr.opensolaris.org/~johnfisc/webserver-design/
http://cr.opensolaris.org/~johnfisc/webserver-design-diff/
http://cr.opensolaris.org/~johnfisc/webserver-design-orig/
Thanks,
John
On 12/13/10 10:06 AM, John Fischer wrote:
All (Ethan, Clay and Joe),
This is a request for a first review of the webserver design project.
I suspect that there will be at least 2 rounds if not more of review.
Please have feedback to me by Friday, December 17th, 2010.
The webrev is located at:
http://cr.opensolaris.org/~johnfisc/webserver-design/
This covers the design as specified in:
http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/AI%2DWebserver%2DSpecification.pdf
I believe it covers the following CRs:
9538 AI webserver not restarting after server reboot
8282 Port sharing with multiple AI webservers
8708 SMF: ai-webserver not restarted if it crashes
8561 Blocker bug for AI Webserver design work
6047 HTTP_PORT should be obtained programmatically, rather than defined
The design is fairly simple. The changes for the design include:
o replaces the cherrypy webserver with a cgi-bin script called
cgi_get_manifest.py.
- the scripts are very similar except without the cherrypy specific code
o replacement of port reference for database with servicename
- an implication of the webserver consolidation, majority of the
important code
changes
o shell scripts no longer start/stop webserver
o compatibility for older services - configuration changes
o build changes - Makefile changes
There are a few additional changes from pep8 checks. I didn't
do the majority of the pylint changes because several of the files
require extensive changes.
Let me know if anyone needs a code walk through.
Thanks,
John
_______________________________________________
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
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
Use shell builtins where possible:
----------------------------------
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#use_true_and_false_ksh93_builtins
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.
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
nit:
----
254 "all_services/default_port = "\
Missing blank between " and \
Inconsistent "<no blank>\
Please clarify:
---------------
255 "integer: 5555'."
Should this be "5555"? I thought we were going to provide default value
configuration.
usr/src/cmd/installadm/delete_service.py 122 lines changed: JJV
usr/src/cmd/installadm/delete_service.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
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.
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.
usr/src/cmd/installadm/installadm.h 2 lines changed: JJV
usr/src/cmd/installadm/installadm.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/installadm/installadm_common.py 37 lines changed: JJV
usr/src/cmd/installadm/installadm_common.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
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
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?
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?
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
