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
55, 70: Please define these as module level private functions, for the
sake of readability and testing.
81: Should be defined as a constant and a tuple
89-94: Should be able to replace with:
mask = ['255'] * (cidr_mask // 8)
102-104: Similarly:
mask.extend(['0'] * (3 - (cidr_mask // 8)))
111, 122: Use "if '/' not in ipv4_one:"
133, 136: Similar comment
133-140: What if they both have different masks?
214, 222, 233: Rewrapping an aimdnserror as an aimdnserror seems very odd.
222, 233: Catching MemoryError seems futile.
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()?
299-302: Nit: Could be: "self.services.setdefault(interface,
[]).append(service)"
414-415: This case seems like it would be covered by the following else
clause; any reason not to just let it do so?
419-421:
for refs in self.sdrefs.get(srv, []):
487: PEP8: Use just "if ... self.verbose" (no "is True")
568: Omit the ">= 0" portion
674: I think ValueError may be more appropriate here.
678, 731, 763, 902, 923, 934, 965, 1112, 1202: Ditch the "is True" portion.
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...)
689-690: Since self.servicename was set to servicename (if non-None),
then just use self.servicename for the rest of the function
825: This should probably be either .append(sdrefs) or .extend(sdrefs)
(no brackets)
922: Just use "if not self.services:"
938: Use "if <X> in <Y>" syntax
1013: The check isn't needed
1017: Deleting while iterating is unstable. The assignment on 1018 is
sufficient.
1109: Better to check "if not args"
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:
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
1170: "commands" is deprecated. Use the subprocess module.
1179-1182: Similarly, use "with" syntax here to ensure file is closed.
1234-1284: Move this to a function called "main()", and call main() on
line 1234. This allows for not having to use globals.
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.
1284: Use the atexit module.
On 11/ 3/10 10:36 AM, John Fischer wrote:
All,
I am still waiting on the code review of aimdns.
Thanks,
John
On 10/28/10 03:13 PM, John Fischer wrote:
All,
I have addressed an issue raised by Clay. The issue was around the SMF
multihomed properties networks and exclude_networks. I have posted a
new webrev and differential webrev at:
http://cr.opensolaris.org/~johnfisc/aimdns-6977107
http://cr.opensolaris.org/~johnfisc/aimdns-6977107-diff
http://cr.opensolaris.org/~johnfisc/aimdns-6977107.orig
Only aimdns.py and test_aimdns.py are effected by this change.
Thanks,
John
On 10/27/10 10:17 AM, John Fischer wrote:
All,
One other thing.... Special thanks to Clay for his pre-webrev
review and help with the test_aimdns.py script. You Rock
Dude!!
Thanks,
John
On 10/27/10 10:11 AM, John Fischer wrote:
All,
I need to do a phased change set for CR 6977107:
usage of dns-sd needs to be replaced within install
as Clay's multihomed project needs to integrate before
I make infrastructure changes to installadm but the
multihomed project needs the dns-sd replacement
(aka aimdns).
So this webrev only includes the changes necessary
to integrate aimdns into slim_source. I have ran the
code through pep8 v0.6.1, pylint v0.18.0 and hg nits.
The webrev is located at:
http://cr.opensolaris.org/~johnfisc/aimdns-6977107/
aimdns uses pybonjour to make mDNS registrations,
browse and lookups for the service records. It consists
of a class and then a python script that uses the class.
I have 2 helper libraries libaimdns.so and netif.so. I
use 2 libraries because the netif.so functionality may be
in a future release of python but the libaimdns.so code
is specific to my needs.
netif.so is a wrapper for the if_nametoindex(3SOCKET)
functions of if_nametoindex(), if_indextoname() and
if_nameindex(). if_nametoindex() specifically is needed
to get the interface index number to pass to pybonjour.
libaimdns.so is a wrapper for getifaddrs(3SOCKET) and
getting various SMF properties via SCF function calls.
There are tests for aimdns, netif.so and libaimdns.so.
I expect to make additional changes to aimdns.py,
probably the tests and the infrastructure within installadm
in phase II (i.e., this isn't the last change set before code
complete).
Feedback appreciated,
John
_______________________________________________
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
_______________________________________________
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
