John,
I never got a chance to review round 1, but here are my comments for
round 2...
installadm-common.sh
-----------------------------
101-107 - How come the C version of this function defaults to a default
port if something fails while reading the smf property, but this shell
version here does not?
server.xml
--------------
100 - Is there a reason why we're naming this "default_port" instead of
just "port" ?
installadm.c
---------------
817-838 - This chunk of code doesn't need to be in a separate set of
brackets anymore.
1339 - nit - tab the second token over instead of spaces.
1408 - Can you elaborate on this comment? It seems like the code is
figuring out whether or not we're dealing with a new service or an old
one and setting path accordingly, not 'ensuring' that we're dealing with
a new one.
installadm_utils.c
---------------------
74,87 - Is this function really yielding a "default" http port number?
It seems to me it's just yielding the actual port number to be used.
Could we remove 'default' from these two lines (and update whatever code
calls this function with its new name)?
89 - Perhaps define a macro named DEFAULT_HTTP_PORT set as '5555' and
use it here as the value instead.
95 - nit - What's the point of initializing 'out' to -1 here?
108 & 109 - nit - could you explicitly compare these to "== -1" or "!=
0" instead?
108 - Do we need to also OR this with "value == NULL"? i.e. could the
function ever return success, but leave value as NULL?
setup-service.sh
---------------------
166 - $2 is described as the data directory here, but it seems like at
line 177, its being treated as the txt record?
183 - typo -- "us" -> "use"
185-189 - If this function is used for when creating a new service, when
would $VARAI/$port already exist?
svc-install-server
----------------------
30,31 - Why not just change the variable names from AIMDNS* to AIMDNSD*
accordingly?
51,54,60 - These comments need to be updated.
89,155 - Where/what is $SMF_PORT set to?
90,156 - Could you make this a more precise grep? For example, using
"Listen ${port}$" to get the exact port number token match and avoiding
a partial hit.
92,158 - Why is 'port_in_config" being set here if its not being used
afterwards at all?
94,160 - Don't we need a space after the "Listen" token?
138 - Doesn't the 'refresh' method also need the equivalent of lines
68-70 from the 'start' method? Either way, there is a large chunk of
identical code here (72-98 and 138-164) that can be moved out to a
separate function and just called from the two places in 'start' the
'refresh' methods.
thanks,
-ethan
On 12/31/10 11:32, John Fischer wrote:
Ethan, Clay, Keith, Dave and Joe,
This is a request for a second review of the webserver design project.
I have updated the project according to the feedback provided.
Clay,
I did not untangle the Thread code within the AI_database.py file as
when I started it I felt that it was more involved then I could do within
the schedule. I will file a CR on the issue and address it that way. Is
that OK with you?
The new webrev is located at:
http://cr.opensolaris.org/~johnfisc/webserver/webserver-design-2/
http://cr.opensolaris.org/~johnfisc/webserver/webserver-design-diff-2/
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.
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
