Hi John,
I had a lot of the same comments as Ethan, but please also update the copyrights
in all of the files.
Sue
On 01/ 3/11 08:24 PM, Ethan Quach wrote:
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
