BIND 10 #2903: need more asio error handling in asiodns

[BIND 10 Development]

#2903: need more asio error handling in asiodns
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  b10-auth      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:14 jinmei]:
 > > I'm not strictly against unrelated cleanups, but this amount seems
 like 50% of the branch. If already written, I don't think it is worth
 throwing it out, or trying to rebuild the history for two branches, but
 the history would be cleaner if the works were separated. So, include it
 now, but let's be more careful about the amount of unrelated work in
 future. Anyway, I think there might be something wrong with out work
 process, if we can't push these cleanups in some fast way without
 cluttering other branches.
 >
 > I'm sorry for the excessive amount of changes.  I admit I tend to be a
 > bad piggy-backer.  For this particular case, in retrospect I should at
 > least have limited the changes to sync_udp_server internal.  As a
 > general workflow, it's probably wise to not do any side work in
 > general, or at least discuss it at the beginning of the review
 > process, before showing the changes.

 Well, my point about the workflow was about the fact that it seems wrong
 there's a motivation for the piggy-backing. It shows that it is too hard
 to push in the small changes that are noticed into master, so they are
 piggy-backed with the bigger tickets as a result. I don't know if there's
 a solution, but it would be nice if it was easy enough to fix these things
 so we don't feel like piggy-backing. Currently, if you want to fix it
 properly, it is either 2-line change or creating a ticket and waiting for
 a long time to get picked up.

 > > Also, a test in distcheck is failing for me. I don't know if it might
 be related, though. Should I try to find the cause of it?
 > > {{{
 > > ...F.......
 > > ======================================================================
 > > FAIL: test_bad_data (__main__.TestUserMgr)
 > > ----------------------------------------------------------------------
 > > Traceback (most recent call last):
 > >   File
 "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/usermgr/tests/b10
 -cmdctl-usermgr_test.py", line 477, in test_bad_data
 > >     'add', 'user1', 'pass1'
 > >   File
 "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/usermgr/tests/b10
 -cmdctl-usermgr_test.py", line 141, in run_check
 > >     self.assertEqual(expected_stdout, stdout.decode())
 > > AssertionError: 'Using accounts file: test_users.csv\nError parsing
 csv file: newline inside str [truncated]... != 'Using accounts file:
 test_users.csv\n'
 > >   Using accounts file: test_users.csv
 > > - Error parsing csv file: newline inside string
 > > }}}
 >
 > I have no idea about this one, and I couldn't reproduce it myself.
 > It also looks quite unrelated to the changes in this branch...does
 > that happen if you run the distcheck on a clean tree (after distclean,
 > or even on a re-cloned repository)?

 I always run the builds and tests on fresh clone of the branch, on a
 separate container (something like lightweight virtual machine) without
 any other network services.

 But I tried on master and it happens too. Maybe I upgraded something and
 the test started to fail. After removing this test, it went through
 correctly. I'm going to create a separate ticket for it.

 '''About the changes''':

 {{{
 % ASIODNS_SYNC_UDP_CLOSE_SOCKET_FAIL_ON_STOP failed to close a
 This is the same to ASIODNS_UDP_CLOSE_SOCKET_FAIL_ON_STOP but happens
 on the "synchronous UDP server", mainly used for the authoritative DNS
 server daemon.
 }}}

 Looking at that, it seems quite a long ID. Even longer than the human-
 readable message. But that might be related to the fact the human-readable
 message looks incomplete.

 Many messages speak about closing when the server is stopping itself. I
 believe we close the sockets under other circumstances too, for example
 when reconfiguring the listening addresses.

 When did we see a failure on close? Under what circumstances can that
 happen? And do we leak a file descriptor in that case, or are we trying to
 close something that is not open?

 From cppcheck (I don't know if they were present before, but it seems
 related):
 {{{
 src/lib/asiodns/sync_udp_server.cc:41: check_fail: Member variable
 'SyncUDPServer::resume_called_' is not initialized in the constructor.
 (warning,uninitMemberVar)
 src/lib/asiodns/sync_udp_server.cc:41: check_fail: Member variable
 'SyncUDPServer::done_' is not initialized in the constructor.
 (warning,uninitMemberVar)
 }}}

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