Keith,
Thanks!! I know this was extra work for you. I truly appreciate it.
John
On 11/ 3/10 04:13 PM, Keith Mitchell wrote:
Hi John,
I skimmed the C files and they seemed ok. I did not have time to
review the test files, but since work is ongoing I assume there will
be opportunity to address any issues there rapidly.
aimdns.py:
General: Was there a reason to use "sys.stdout.write(...)" over "print
..."? Just curious, I think it's ok either way
I used print in list.py but was told to use sys.stdout.write(). Our
code seems to use a variety
of output mechanisms:
print
print >>sys.stdout
print >>sys.stderr
sys.stderr.write
I'll change these to the print format. Yikes, lots of 'em.
55, 70: Please define these as module level private functions, for the
sake of readability and testing.
Pulled up and out. Will add a test for these as well.
81: Should be defined as a constant and a tuple
OK.
89-94: Should be able to replace with:
mask = ['255'] * (cidr_mask // 8)
Cool.
102-104: Similarly:
mask.extend(['0'] * (3 - (cidr_mask // 8)))
Cool.
111, 122: Use "if '/' not in ipv4_one:"
133, 136: Similar comment
OK.
133-140: What if they both have different masks?
Good question. So these checks are seeing if the IPv4 addresses can
be considered that they are on the same network. Thus the binary &
for each mask on each IPv4 address.
It was decided that checking the IP1 & mask2 against IP2 & mask2
was the correct thing to do. Thus the mask1 portion of the code
has been removed.
214, 222, 233: Rewrapping an aimdnserror as an aimdnserror seems very
odd.
Will unwrap.
222, 233: Catching MemoryError seems futile.
OK.
250: Busy waiting seems bad. Can you use something like a
threading.Event() that supports a wait() call?
252: "reset()" seems an odd thing to do here, is there any reason not
to just clear_sdrefs()?
Sure.
299-302: Nit: Could be: "self.services.setdefault(interface,
[]).append(service)"
Cool.
414-415: This case seems like it would be covered by the following
else clause; any reason not to just let it do so?
That's funny. I originally wrote it that way. I was hoping that this
would speed things
up a little bit. But it adds complexity so I'll just remove it.
419-421:
for refs in self.sdrefs.get(srv, []):
Holy Molly. That's cool.
487: PEP8: Use just "if ... self.verbose" (no "is True")
Yep. I have replaced stuff like that before.
568: Omit the ">= 0" portion
Corrected.
674: I think ValueError may be more appropriate here.
Sounds right.
678, 731, 763, 902, 923, 934, 965, 1112, 1202: Ditch the "is True"
portion.
Yep.
685, 803: This doesn't seem the best place to raise a SystemExit.
Probably best to just strip the try/except and bubble the SystemError
(which ideally changes to something other than "SystemError" at some
point...)
This is copied code from elsewhere within AI stuff. I think I
originally pulled this from the ai-webserver
for installadm list. Here is a snippet from an interactive shell:
>>> import osol_install.libaiscf as smf
>>> smf.AISCF(FMRI="system/install/server1")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
SystemError: entity not found
Yep, I know that the service name is incorrect. When I didn't have the
code within a try/except
block like this I was told that I needed to catch it from a previous
webrev. What should I do here?
689-690: Since self.servicename was set to servicename (if non-None),
then just use self.servicename for the rest of the function
Yep. Not sure why I did that one. :-/
825: This should probably be either .append(sdrefs) or .extend(sdrefs)
(no brackets)
Yep. This was caused from playing around with adding a string to a list
via extend.
lst = ['abc', 'efg']
lst.extend('hij')
print lst
['abc', 'efg', 'h', 'i', 'j']
But I am not dealing with strings. So the no brackets will work.
922: Just use "if not self.services:"
If you can't tell.... that just confuses me. Oye. I've changed it.
938: Use "if <X> in <Y>" syntax
Actually, I am going to use string.startswith() syntax instead. I think
Clay
thought that we might have a case where a new client might contain this
syntax as well.
1013: The check isn't needed
OK.
1017: Deleting while iterating is unstable. The assignment on 1018 is
sufficient.
OK.
1109: Better to check "if not args"
Some habits are hard to break.
1119: This could get complicated fast (if another exclusive option is
added). Recommend changing to:
if [bool(loptions.browse),
bool(loptions.all),
bool(loptions.register),
bool(loptions.service)].count(True) > 1:
WOW. That's tricky.
1167: If the int() call fails here, the file won't be close on line
1168. Use the "with" syntax to open/close files with ease:
with open(PIDFILE) as pidfile:
# get pid from file
OK. Didn't know that syntax either.
1170: "commands" is deprecated. Use the subprocess module.
Will do.
1179-1182: Similarly, use "with" syntax here to ensure file is closed.
OK.
1234-1284: Move this to a function called "main()", and call main() on
line 1234. This allows for not having to use globals.
OK.
1280: At minimum, print out the error. If this only occurs as a result
of a programming bug (i.e., it's abnormal), print a full traceback as
well.
Not sure what to code here. I'll think about what is appropriate.
1284: Use the atexit module.
OK.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
