Hey John,
My comments are attached. Some are just questions.
Hope this helps.
Joe
On 11/ 3/10 01:36 PM, 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
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/Makefile 1 line
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
59 PYTHON_MODULES= delete_service.py delete_client.py create_client.py aimdns.py
I think aimdns should go with the utility code, same plase as
installadm_common.py and not the sub-commands
e.g.:
55 PYMODULES= installadm_common.py aimdns.py
56
57 PYTHON_EXECS= list
58
59 PYTHON_MODULES= delete_service.py delete_client.py create_client.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/cmd/installadm/aimdns.py 1284 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
In addition to the comments Keith made...
General Issue/Question:
------------------------
Question:
---------
Are you re-inventing the Python IP address wheel? ;) and what about IPv6?
Would it be possible to leverage the work being done for Python
PEP 3144 -- IP Address Manipulation Library for the Python Standard Library
http://www.python.org/dev/peps/pep-3144/
There is a Reference Implementation provided:
http://ipaddr-py.googlecode.com/svn/trunk
Issue:
------
159 # FMRI for AI service and select properties, private
160 _SRVINST = 'svc:/system/install/server:default'
161 _EXCLPROP = 'all_services/exclude_networks'
162 _NETSPROP = 'all_services/networks'
163 _PORTPROP = 'config/port'
164
165 # Default port for the webserver
166 _DEFAULT_PORT = 5555
167
These should probably be made available in a AI common module as
other code will need it too?
Perhaps installadm_common.py or a new installadm_defines.py or ... ?
This should likely be handled by your planned additional changes
to add to the installadm infrastructure.
Issues:
-------
I had thought we were not listing the pylint disable-msg exceptions ?
90 # pylint: disable-msg=W0612
91 # pylint complains about 'count' being unused
106 # pylint: enable-msg=W0612
210 # pylint: disable-msg=E1101
211 # libaimdns has aiMDNSError but pylint reports otherwise
213 # pylint: enable-msg=E1101
220 # pylint: disable-msg=E1101
221 # libaimdns has aiMDNSError but pylint reports otherwise
223 # pylint: enable-msg=E1101
231 # pylint: disable-msg=E1101
232 # libaimdns has aiMDNSError but pylint reports otherwise
234 # pylint: enable-msg=E1101
254 # pylint: disable-msg=W0613
306 # pylint: enable-msg=W0613
308 # pylint: disable-msg=W0613
373 # pylint: enable-msg=W0613
460 # pylint: disable-msg=W0613
492 # pylint: enable-msg=W0613
574 except libaimdns.aiMDNSError, err: # pylint: disable-msg=E1101
708 # pylint: disable-msg=W0613
776 # pylint: enable-msg=W0613
1207 # pylint: disable-msg=W0613
1230 # pylint: enable-msg=W0613
Issue:
------
199 self.port = 0
993 self.port = 0
Where is this self."port" attribute used? I'm a little confuesed as
to what "port" attribute represents what...
Perhaps a more descriptive attribute name, self.srv_port vs smf_port
and or a comment.
Issue/Question:
------
250 while not self._stopped:
Would it make sence to code in a max time to wait?
e.g.: Maybe something like this:
max=0
while not self._stopped and max < SOME_BIG_NUMBER:
max += 1
continue
if max == SOME_BIG_NUMBER:
raise AIMDNSError(_('error:... waited too long... bla bla bla...
Issue:
------
Don't need 'is True'
448 if self._do_lookup is True:
487 self.verbose is True:
608 if self.verbose is True:
678 if self.verbose is True:
731 if self.verbose is True:
763 if self.verbose is True:
799 if self.verbose is True:
859 if self.verbose is True:
902 if self.verbose is True:
923 if self.verbose is True:
934 if self.verbose is True:
1112 if loptions.all is True or loptions.register is not None:
1202 if REMOVE_PID is True:
e.g.:
if self._do_lookup:
...
Issue:
-----
1119 if (loptions.browse and loptions.all) or \
1120 (loptions.browse and loptions.register) or \
1121 (loptions.browse and loptions.service) or \
1122 (loptions.all and loptions.register) or \
1123 (loptions.all and loptions.service) or \
1124 (loptions.register and loptions.service):
1125 parser.error(_('"-f", "-b", "-r" and "-a" operations are mutually '
1126 'exclusive.'))
Scary! ;)
Maybe add a commnet and break this up into multiple checks.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/cmd/installadm/test/test_aimdns.py 246 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Since you plan to make more changes including testing I haven't
spent a lot of time looking this over.
What did you get for code coverage numbers?
For example using:
pfexec nosetests --with-coverage ./test_aimdns.py
Question:
---------
It's not clear to me what value these classes add:
38 class RegisterAPI(unittest.TestCase):
107 class FindAPI(unittest.TestCase):
212 class TestCompareIPv4(unittest.TestCase):
Please explain.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/Makefile 3 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/libaimdns/Makefile 87 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/libaimdns/libaimdns.c 867 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Suggestion:
It seems you are using buf to hold the property value and to
pass an error to cleanup.
It may be simpler to use a char* err_buf for the errors.
Could avoid the snprintf call and focus multiple error message
in one part of the source. This could aid in i18n-ing and changing
maintenance.
Define test message strings:
94 snprintf(buf, BUF_MAX, "unable to create SCF handle");
100 snprintf(buf, BUF_MAX, "unable to bind to the service
daemon");
106 snprintf(buf, BUF_MAX, "unable to create the property");
117 snprintf(buf, BUF_MAX, "unable to decode the property:%s
%s",
125 snprintf(buf, BUF_MAX, "unable to create the value");
...
e.g.:
#define ERROR_SCF_HANDLE "unable to create SCF handle"
...
err_buf = ERROR_SCF_HANDLE;
...
Issue:
------
140 rtn = strdup(buf);
I believe this is creating a memory leak.
It might be best to have the caller provide the buffer,
which could defined as static.
e.g.:
From:
181 char *value[BUF_MAX];
To
char value_buff[BUF_MAX];
char* value;
value = value_buff;
Then:
From:
188 value = get_astring_property(fmri, propname);
To
get_astring_property(fmri, propname, &value);
and have get_astring_property populate value without doing the strdup()
Issue:
-------
Use gettext around error strings for i18n
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/libaimdns/mapfile-vers 49 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Seems OK...
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/libaimdns/test/test_libaimdns.py 143 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Since you plan to make more changes including testing I haven't
spent a lot of time looking this over.
What did you get for code coverage numbers?
For example using:
pfexec nosetests --with-coverage ./test_aimdns.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/netif/Makefile 88 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
seems OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/netif/mapfile-vers 49 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
seems OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/netif/netif.c 252 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
-------
Use gettext around error strings for i18n
Issue:
------
Consider #define-ing string constants.
e.g.:
138 "no interface with this name");
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/netif/test/test_netif.py 64 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Since you plan to make more changes including testing I haven't
spent a lot of time looking this over.
What did you get for code coverage numbers?
For example using:
pfexec nosetests --with-coverage ./test_aimdns.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/pkg/manifests/install-installadm.mf 3 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
87 file path=usr/lib/installadm/aimdns
I'm not sure this is the best place for this. I think it should be with
the autoinstall/installadm utils... but we can work that out in the ISIM
project.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
