BIND 10 #299: AXFR fails half the time

[BIND 10 Development]

#299: AXFR fails half the time
-----------------------------+----------------------------------------------
      Reporter:  zzchen_pku  |        Owner:  zzchen_pku
          Type:  defect      |       Status:  reviewing 
      Priority:  major       |    Milestone:            
     Component:  xfrout      |   Resolution:            
      Keywords:              |    Sensitive:  0         
Estimatedhours:  0.0         |        Hours:  0         
      Billable:  1           |   Totalhours:  1.5       
      Internal:  0           |  
-----------------------------+----------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => zzchen_pku


Comment:

 Replying to [comment:33 zzchen_pku]:
 > unittests have been updated.
 >
 okay, confirmed.  I've noticed there are obsolete comments (due to the
 change of this branch) in the test code.  I've directly made a change on
 the branch to remove them (r3435).

 Now going back to the main patch:

 '''xfrout.py'''

  - I don't understand why we need to check _shutdown_event in
 !XfroutSession.handle():
 {{{
         while not self.server._shutdown_event.is_set(): # Check if xfrout
 is shutdown
 }}}
   when shutdown happens, shouldn't the !XfroutSession be able to detect
 that via the _shutdown_sock?
  - when EINTR happens, recv_fd() will be called even if it may not be
 readable.
  - why can't this be log but stderr?
 {{{
                     sys.stderr.write("[b10-xfrout] Error with select():
 %s\n" %e)
 }}}
  - (this is not specific to this patch but anyway) I'm not sure if it's
 guaranteed all data is received here:
 {{{
             msgdata = self.request.recv(msg_len)
 }}}
   isn't it possible (at least in theory) that it's a partial read?
  - naively assuming IPv4 is not good:
 {{{
             sock = socket.fromfd(fd, socket.AF_INET, socket.SOCK_STREAM)
 }}}
   although as far as I know the second parameter of fromfd will be simply
 ignored.
  - I suspect the naming of "_{master,slave}_sock" is confusing:
 {{{
         self._master_sock, self._slave_sock = socket.socketpair()
 }}}
   because it may look like related to the notion of DNS master (primary)
 and slave (secondary) while it is not.

 '''fd_shar_python'''
  - I was not sure what "REV" in "XFR_FD_REV_FAIL" means (receive?).  I'd
 suggest not to be afraid of using relatively long variable name if it
 clarifies the intent.
  - using static this way is not C++-ish:
 {{{
 static PyObject *XFR_FD_REV_FAIL;
 }}}
   as far as remember this usage is actually even "deprecated".  (although
 this type of cleanup may better be deferred to a different ticket)
  - also please add documentation of this constatnt (at least in C++, and
 if possible in the python wrapper)
  - error cases should be handled properly here:
 {{{
     XFR_FD_REV_FAIL = Py_BuildValue("i", isc::xfr::XFR_FD_REV_FAIL);
     PyModule_AddObject(mod, "XFR_FD_REV_FAIL", XFR_FD_REV_FAIL);
 }}}

 '''Other high level points'''

 > I think we need to create a new ticket for this:-), or this ticket will
 become more complex. I'll create one if you agree.
 >
 That's fine.

 For that matter, I've noticed test coverage of xfrout is not really good
 (my test showed only 57% of the code is covered).  We'll also need a
 separate task (ticket) to improve test coverage.

-- 
Ticket URL: <http://bind10.isc.org/ticket/299#comment:34>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development