Joe,
I'll have a new webrev out later today with the changes from yourself,
Keith and Clay
included. See below for details.
Thanks,
John
On 11/ 4/10 12:19 PM, [email protected] wrote:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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
-- this is both a class as well as an executable more like list.py and
installadm_common.py combinded.
So it should be in the PYTHON_MODULES otherwise it ends up in
usr/lib/python2.6/vendor_packages/\
osol_install/ instead of usr/lib/installadm.
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?
-- Yes and no. Essentially I saw the ipaddr.py stuff on googlecode while
doing research and trying to fix various issues within the code.
Believe
me it would have been great to use this type of thing instead of home
grown. The issue is that ipaddr.py is not in our product thus it would
require Oracle Legal review for inclusion. Also it doesn't do what we
need it to do.
-- IPv6 Issue: AI is not currently dealing with IPv6 only IPv4.
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?
-- that makes sense. Added to installadm_common.py
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.
-- OK.
Issues:
-------
I had thought we were not listing the pylint disable-msg exceptions ?
-- Not a clue if we are or are not. Some of these have gone away now anyhow
with the comments from Keith.
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.
-- This is the service port number. It is either passed in or retrieved
from the service SMF properties. The context is the mDNS registered
service thus the self.port seems to make sense here.
Issue/Question:
------
250 while not self._stopped:
Would it make sence to code in a max time to wait?
-- I needed that because of the test. I have removed it because I've
changed
a couple of other things within the test and aimdns.py. Not an
issue now.
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'
-- Yep. I think I've found all of these from Keith's comments.
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:
-----
-- Keith caught that one too. I've made the change according to Keith's
suggestion.
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?
-- I did not know this was a requirement. nosetests is an opensource
project. Has it had Oracle Legal review? I've installed both nosetests
and coverage (both opensource needing Oracle Legal review). I've added
a couple of new tests which showed 50% coverage for aimdns.py. It is
actually higher then that as the original tests check the main
functionality
of registration, find, browse, and compare_ipv4. These
functionality cover
the majority of the code.
libaimdns and netif showed 100% test coverage
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.
-- The way that unittest works is by creating classes subclassed off of
unittest.TestCase for the execution of the actual test. The subclasses
within each test harnse are used to establish the dummy environment.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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:
-- Corrected with #define in a private header file and
gettext(#define-identifier)
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:
------
-- Corrected by still doing the strdup() logic but prior to returning to
python value is freed.
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:
-------
-- Using gettext around error strings.
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?
-- 100% for this module
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:
-------
-- Corrected.
Use gettext around error strings for i18n
Issue:
------
Consider #define-ing string constants.
-- Corrected.
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?
-- 100% for this module
For example using:
pfexec nosetests --with-coverage ./test_aimdns.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/pkg/manifests/install-installadm.mf 3 lines
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
-- OK
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
