Hi John,
You're welcome! Responses inline, some items with no responses removed.
- Keith
On 11/ 4/10 12:22 PM, John Fischer wrote:
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.
Yeah, I don't think we have a standard for that. If you haven't already
changed it, I don't feel to strongly that it needs to change, so feel
free to skip this if there's any time concerns.
[...]
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.
Ok, that makes sense.
[...]
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.
[...]
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?
Ah, in comparing the messages it looks like the better error message is
valuable. Feel free to catch it in an "except" clause; the main point
I'd like to see changed is raising something other than SystemExit.
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. :-/
Seems like the sort of thing that happens during developmental
refactoring! I've ended up with similar odd dangling bits of code...
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.
If you're trying to add a single item (like in the string case above),
you want to use append. If you're "copying" all items for a list (or
iterable) to the end of this list, you want extend. I just wanna make
sure that the right function is being used here.
922: Just use "if not self.services:"
If you can't tell.... that just confuses me. Oye. I've changed it.
Slightly confusing, majorly advantageous! Checking in this fashion
catches cases where "self.services" is empty, and also the case where
"self.services is None".
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.
Even better.
[...]
1109: Better to check "if not args"
Some habits are hard to break.
Yes they are! In the general case of these checks, there's the
(theoretical) danger of 'args' being an empty tuple, which is actually
not equal to an empty list, but still just as empty.
[...]
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.
Yeah, I just want to make sure we're not exiting silently in the error case.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
