Ethan and Sue,
Thanks, great feedback. I appreciate the help.
In addition to your comments I have updated the copyright dates
for all files concerned. See below.
I'll produce an updated webrev shortly. I am waiting on the ON
gatekeepers to be sure that I get the copyright changes correct.
Once that happens I'll provide a new webrev along with a differential
webrev.
Thanks,
John
On 01/ 3/11 08:24 PM, Ethan Quach wrote:
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?
Changed to:
function get_http_port
{
HTTP_PORT=$($SVCPROP -cp all_services/port \
$SMF_INST_SERVER 2>/dev/null)
ret=$?
if [[ "X$HTTP_PORT" == "X" ]] ; then
HTTP_PORT=5555
fi
return $ret
}
server.xml
--------------
100 - Is there a reason why we're naming this "default_port" instead
of just "port" ?
No. Changed in installadm-common.sh, installadm.h, installadm_common.py
as well.
installadm.c
---------------
817-838 - This chunk of code doesn't need to be in a separate set of
brackets anymore.
Corrected.
1339 - nit - tab the second token over instead of spaces.
Tabbed over.
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.
Replaced:
/*
* Ensure we're dealing with a new service setup
*/
With:
/*
* The new server setup is located at var/ai/. The new
* service setup adds the service name to the end of this
* path. First, create a path assuming that we are dealing
* with a new service setup. Then ensure that this path
* exists and use it if it does. Otherwise, default to
* an older service setup that uses the port instead of
* the service name.
*/
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)?
Removed default from the name. Also updated installadm.c,
installadm-common.sh
and setup-sparc.sh.
89 - Perhaps define a macro named DEFAULT_HTTP_PORT set as '5555' and
use it here as the value instead.
Updated. Added to installadm.h:
#define DEFAULT_HTTP_PORT 5555
95 - nit - What's the point of initializing 'out' to -1 here?
Actually, none. Removed.
108 & 109 - nit - could you explicitly compare these to "== -1" or "!=
0" instead?
Comparing explicitly != 0.
108 - Do we need to also OR this with "value == NULL"? i.e. could the
function ever return success, but leave value as NULL?
I checked with the folks on #smf-dev and they said no.
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?
Yep.
183 - typo -- "us" -> "use"
Corrected.
185-189 - If this function is used for when creating a new service,
when would $VARAI/$port already exist?
Right. There is no reason. If it exists then that would be a problem.
I have added
an else clause to the if statement which returns -1 (i.e., error).
svc-install-server
----------------------
30,31 - Why not just change the variable names from AIMDNS* to
AIMDNSD* accordingly?
Updated.
51,54,60 - These comments need to be updated.
Added daemon to the comment
89,155 - Where/what is $SMF_PORT set to?
Dang it. Where did that go? It is supposed to be set to:
SMF_PORT=$($SVCPROP -cp all_services/port ${INSTANCE} 2>/dev/null)
Corrected.
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.
Corrected.
92,158 - Why is 'port_in_config" being set here if its not being used
afterwards at all?
Removed. I was using it in a different iteration.
94,160 - Don't we need a space after the "Listen" token?
Yes. The echo statement handles that with the space after the double quote.
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.
Moved to a function with the above changes.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
