On 3/1/2011 11:03 AM, Keith Mitchell wrote:
On 03/ 1/11 10:11 AM, Martin Widjaja wrote:
Sue - In reviewing this quickly, it looks good overall, but I have several
questions, mostly to satisfy my own curiosity:
I know you asked Sue, but I have some answers to chime in with here.
- Makefile: would it be worth keeping the generic rules for building .c, etc.?
Maybe this can be useful for other things later on?
- installadm.py: 354: is there a reason to remove the traceback limit here? Is
this going to be too much if everything is printed?
If we reach a point where we're printing a traceback, we're already in a
scenario where the user encountered a bug. More information is better than
less, in that case (and the limit=2 almost always cut out the useful portion of
the traceback, since it cuts out the 'bottom' of the stack).
- installadm_common.py: 95-105: is this ksh execution to determine multi-home
going to persist or is there a better way to gather this information using
either python lib or C lib? I am personally not a fan of command line output
parsing.
I was thinking of swapping that out with ISIM, though it may be quick to change
as part of this push. The concern I have with doing it *right* now is it would
be easy to miss edge cases - by porting this right across from the .c
implementation, we're at least not introducing any new bugs.
OK. Sure, that's a good strategy.
- libaiscf.py: 105: what's wrong with previous implementation?
The old version was simply broken - there was an extraneous "self" argument.
Any attempt to call that function resulted in a traceback.
OK. I guess you cannot really call a function with "self", and that's fine, but
wouldn't something like this works:
return (AIservice(super(AISCF, self).new_service(service_name)))
I guess the part I don't really like/understand with having the call to super
class (which seems to be defined in C code libaiscf_instance.c?) is that it's
unclear how the AISCF.new_service gets the return value from that superclass
call, and unless the service_name is actually populated with a value, it's not
too obvious what the return value here is.
If we explicitly return the whole thing like above (assuming superclass
actually return something), it is easier to understand.
But then again... maybe I missed something here.
Thanks,
Martin
Thanks,
Martin
PS. test_ai_smf_service.py: can't get more optimistic than wobbly knees and
failing eyesight..
On 2/28/2011 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
