Hi John,
Thank you for the review.
On 03/ 1/11 04:14 PM, John Fischer wrote:
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.
The goal is to eventually have only ai_smf_service interface with libaiscf (or
eventually rad, from the smf team) and for modules to import ai_smf_service as
aismf. The rest of the conversion will be done as part of the isim project and
not in this set of changes.
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.
Most of these changes were simply changing the import name from smf to libaiscf,
but in any case, these interfaces will be changing to use rad so I'd rather
leave them for now.
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.
Will change these to be more 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?
I think the message is fine as it is. Someone who knows enough to set up a dhcp
server would be able to spot the error if the entire ip address is shown.
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.
It is correct, We want the error to be caught higher up so we can exit. I will
add comments to clarify.
get_image_arch(path) is a good candidate for being moved into the
installadm_common.py file as list.py could also use it.
Will move 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.
Good point. Will change to use a set as Keith suggested in the other email.
Is line 364 needed as line 376 resets it and it is never used in between:
Yes, a leftover, will remove.
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/'
ok
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.
Will beef up comments.
================================================================================
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
Thanks again for the review, John.
Updated webrev posted at:
http://cr.opensolaris.org/~sohn/7020631.2
http://cr.opensolaris.org/~sohn/7020631.2.diffs
Sue
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
