Sue,
Lots of good stuff going on. Awesome.
John
================================================================================
usr/src/cmd/Makefile
================================================================================
looks good
================================================================================
usr/src/cmd/installadm/Makefile
================================================================================
looks good
================================================================================
usr/src/cmd/installadm/ai_smf_service.py
================================================================================
other parts of installadm import libaiscf as smf. All the files should
be consistent. Specifically:
ai_smf_service.py
aimdns_mod.py
create_client.py
delete_service.py
installadm_common.py
list.py
Might need to change.
Several functions repeatedly call
libaiscf.AISCF(FMRI=AI_SVC_FMRI).<something>.
Why not do the following instead:
smf_inst = libaiscf.AISCF(FMRI=AI_SVC_FMRI)
smf_inst.<something1>
smf_inst.<something2>
Examples of this is at lines 163, 165, 167, 230, 234, 270, 274, 305,
309, 340, and 344.
Several locations use libaiscf.AISCF() instead of
libaiscf.AISCF(FMRI=AI_SVC_FMRI). Why
the difference? Everytime I did that someone would tell me to be explicit.
================================================================================
usr/src/cmd/installadm/create-service.c
================================================================================
makes sense
================================================================================
usr/src/cmd/installadm/create_client.py
================================================================================
looks good
================================================================================
usr/src/cmd/installadm/create_service.py
================================================================================
Would it make sense to explicity state which octet was malformed in
check_ip_address(option, opt_str, value, parser) at lines 64 and 66?
Can:
214 Popen.check_call([com.SETUP_DHCP_SCRIPT, com.DHCP_SERVER,
ip_start,
215 str(ip_count)])
return a CalledProcessError exception? This exception is caught for the
Popen.check_call at line 228.
get_image_arch(path) is a good candidate for being moved into the
installadm_common.py file as list.py could also use it.
The consolidated webserver services all use port 5555. Compatibility
services
uses unique service port numbers starting at 46501. The following code:
279 existing_ports = []
280 for name in service_instance.services:
281 service = libaiscf.AIservice(service_instance, name)
282 svckeys = service.keys()
283 if aismf.PROP_TXT_RECORD in svckeys:
284 port = service[aismf.PROP_TXT_RECORD].split(':')[-1]
285 existing_ports.append(int(port))
Will append port 5555 for each non-compatibility service. Perhaps a check
if the port is the default port then do not append it. If you do this then
the existing_ports = [] should be changed to have 5555 initially.
Is line 364 needed as line 376 resets it and it is never used in between:
364 compatibility_port = False
...
376 compatibility_port = bool(Popen(cmd).wait())
================================================================================
usr/src/cmd/installadm/installadm.h
================================================================================
makes sense
================================================================================
usr/src/cmd/installadm/installadm.py
================================================================================
looks good.
================================================================================
usr/src/cmd/installadm/installadm_common.py
================================================================================
It might be good to add a comment for lines 67, 71 and 73.
67 AI_NETIMAGE_REQUIRED_FILE = "solaris.zlib"
...
71 WANBOOTCGI = 'cgi-bin/wanboot-cgi'
...
73 AI_SERVICE_DIR_PATH = '/var/ai/'
Kind of a toss up where to put the MULTIHOMED_TEST variable and
is_multihomed
function. Seems odd to put the function above the classes when all over
functions are below the classes. But with it near the variable definition
it is easier to see what is being used. So I am OK with the placement.
Once
the is_multihomed function is converted to use python exclusively then the
function should be moved down with the other functions below the classes.
A little more comments might be good around:
1317 # accept alphanumeric chars, '-', and '_'
1318 svcname = svcname.replace("-", "").replace("_", "")
1319 if not svcname.isalnum():
to explain why the code is riping out - and _ from the service name.
The new
code looks way more efficient.
================================================================================
usr/src/cmd/installadm/installadm_util.c
================================================================================
makes sense
================================================================================
usr/src/cmd/installadm/smf_test/test_ai_smf_service.py
================================================================================
looks good, almost commented on the cls.name3 and cls.pgname3 but I see
where
those are used later on.
================================================================================
usr/src/cmd/installadm/test/installadm_test.c
================================================================================
makes sense
================================================================================
usr/src/lib/libaiscf_pymod/libaiscf.py
================================================================================
looks good
================================================================================
usr/src/pkg/manifests/install-installadm.mf
================================================================================
looks good
On 02/28/11 11:44 AM, Sue Sohn wrote:
Please do a code review of the changes for:
7020631 Convert create-service.c to python
http://monaco.sfbay.sun.com/detail.jsf?cr=7020631
Webrev:
http://cr.opensolaris.org/~sohn/7020631
Thanks,
Sue
_______________________________________________
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
