BIND 10 #2225: Implement counters into Xfrout (3/3)

[BIND 10 Development]

#2225: Implement counters into Xfrout (3/3)
-------------------------------------+-------------------------------------
            Reporter:  naokikambe    |                        Owner:
                Type:  enhancement   |  naokikambe
            Priority:  medium        |                       Status:
           Component:  xfrout        |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  7             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => naokikambe


Comment:

 Hi Kambe san

 Here is my review of the `trac2225_xfrout` branch.

 * In `xfrin.py.in`, Can't the following code be simplified? The replace
   method calls assume that `DATAROOTDIR` has the word `share` in it
   anyway. BTW, does this original modification have any reason to be in
   the `trac2225_xfrout` branch?
 {{{
 # If B10_FROM_BUILD or B10_FROM_SOURCE is set in the environment, we
 # use data files from a directory relative to that, otherwise we use
 # the ones installed on the system
 PREFIX = "@prefix@"
 DATAROOTDIR = "@datarootdir@"
 if "B10_FROM_BUILD" in os.environ:
     AUTH_SPECFILE_PATH = os.environ["B10_FROM_BUILD"] + "/src/bin/auth"
 else:
     AUTH_SPECFILE_PATH = "@datadir@/@PACKAGE@".replace("${datarootdir}",
 DATAROOTDIR).replace("${prefix}", PREFIX)
 if "B10_FROM_SOURCE" in os.environ:
     SPECFILE_PATH = os.environ["B10_FROM_SOURCE"] + "/src/bin/xfrin"
 else:
     SPECFILE_PATH = "@datadir@/@PACKAGE@".replace("${datarootdir}",
 DATAROOTDIR).replace("${prefix}", PREFIX)
 SPECFILE_LOCATION = SPECFILE_PATH + "/xfrin.spec"
 AUTH_SPECFILE_LOCATION = AUTH_SPECFILE_PATH + "/auth.spec"
 }}}

 Instead of this, we can use something like:
 {{{
 # If B10_FROM_BUILD or B10_FROM_SOURCE is set in the environment, we
 # use data files from a directory relative to that, otherwise we use
 # the ones installed on the system
 PREFIX = "@prefix@"
 SPECFILE_PATH = "@prefix@/share/@PACKAGE@"
 AUTH_SPECFILE_PATH = SPECFILE_PATH
 if "B10_FROM_BUILD" in os.environ:
     AUTH_SPECFILE_PATH = os.environ["B10_FROM_BUILD"] + "/src/bin/auth"
 if "B10_FROM_SOURCE" in os.environ:
     SPECFILE_PATH = os.environ["B10_FROM_SOURCE"] + "/src/bin/xfrin"
 SPECFILE_LOCATION = SPECFILE_PATH + "/xfrin.spec"
 AUTH_SPECFILE_LOCATION = AUTH_SPECFILE_PATH + "/auth.spec"
 }}}

 * Is the same change (above) required in `xfrout.py.in` where
   `B10_FROM_SOURCE` doesn't seem to be used?

 * In the code, as an example, `socket/unixdomain/senderr` is
   incremented. But the `b10-xfrout` manpage does not mention
   `socket/unixdomain/`.

 * In `UnixSockServer.__init__()`, what happens now if
   `ThreadingUnixStreamServer.__init__()` raises an exception?
 {{{
 -        ThreadingUnixStreamServer.__init__(self, sock_file, handle_class)
 +        self._counters = Counters(SPECFILE_LOCATION)
 +        try:
 +            ThreadingUnixStreamServer.__init__(self, sock_file, \
 +                                                   handle_class)
 +        except:
 +            self._counters.inc('socket', 'unixdomain', 'openfail')
 +        else:
 +            self._counters.inc('socket', 'unixdomain', 'open')
 }}}

 Should it not raise the exception again after updating the counter?
 There should be an `assertRaises` check in the corresponding testcase
 too (in `test_open()`).

 * Here, why is `close` incremented inside the `try` block, when it's
   already shutdown outside the try block?
 {{{
     def shutdown(self):
         self._write_sock.send(b"shutdown") #terminate the xfrout session
 thread
         super().shutdown() # call the shutdown() of class
 socketserver_mixin.NoPollMixIn
         try:
             # count closed unixsockets
             self._counters.inc('socket', 'unixdomain', 'close')
             os.unlink(self._sock_file)
         except Exception as e:
             logger.error(XFROUT_REMOVE_UNIX_SOCKET_FILE_ERROR,
 self._sock_file, str(e))
 }}}

 * A couple of `NotifyOut` tests seem to have been removed. Why?

 * Is there anything left to be done for this unittest:
 {{{
     def test_senderr(self):
         # do inside of above TestXfroutSession
         pass
 }}}

 * I have not tested the lettuce changes as I have issues running lettuce
   on my box with Python 3.3 currently. I'll do this during the next
   round of review.

 * I've also pushed some minor patches. Please check them.

-- 
Ticket URL: <https://bind10.isc.org/ticket/2225#comment:42>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development