BIND 10 #1924: Msgq undeliverable notifications

[BIND 10 Development]

#1924: Msgq undeliverable notifications
-------------------------------------+-------------------------------------
            Reporter:  vorner        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  msgq          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130219
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  6             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:  msgq-
                                     |  ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:12 vorner]:

 > I went with slightly simpler approach. I don't define logical
 parameters, just constants and I generate the header and python code from
 the cc file. This should ensure the values are the same.
 >
 > I used it in the modified methods, leaving the others for some follow-up
 cleanup task. Is that OK?

 That's fine.

 > I tried to address that. But that's not bullet-proof, since we can still
 get the EPIPE afterwards when the message is in the queue. Tracing that
 would take a lot more work than this and it should be rather rare I think.
 >
 > > {{{#!python
 > >             # [..] We need to send
 > >             # an error to satisfy the senders hurger for response,
 since
 > >             # nobody else will do that.
 > > }}}
 > >   At least I didn't understand what "hurger" means (can't find it in
 > >   my dictionary, and have no idea of what it would be if it were a
 > >   typo).
 >
 > It should have been hunger.

 In that case I believe it should be "the senders that hunger for
 response".

 > > - If msgq registers itself, doesn't it have its own lname?
 > > {{{#!python
 > >             header["from"] = 'msgq' # Dummy lname not assigned to
 clients
 > > }}}
 >
 > I don't think if that would work. After all, msgq connects to itself
 only after the config manager appears, so there's time when it doesn't
 have any lname assigned. Also, it is not exactly correct if we use that,
 because the lname represents a connection and this is the daemon talking,
 not the connection (it wouldn't be possible to distinguish the daemon from
 the msgq cc connection). And it could make debugging slightly easier, if
 it is clearly visible.
 >
 > Should I describe it or try to use the lname if it is available, or
 something?
 >
 > Actually, I think the lname is not needed at all at that time, because
 it's only for msgq addressing. I think we could omit it entirely, but I
 don't want to do that in case some debug code or something assumed it is
 there and crashed unexpectedly.

 This is a minor point, and I'm okay with the current implementation.

 > > - as discussed in a different ticket, we should have consensus on the
 > >   use of `ntohl/ntohs`:
 > > {{{#!cpp
 > >         const uint32_t total_len = ntohl(total_len_data);
 > >         const uint16_t header_len = ntohs(header_len_data);
 > > }}}
 > >   But, in this particular case, it seems possible to avoid that
 > >   discussion by using `util::InputBuffer`, which would still keep
 > >   the code reasonably understandable.
 >
 > It is not there any more. But there are hton[ls] in the code I didn't
 change. Should I update that, or do we wait if it breaks somewhere? (it
 didn't break for some very long time now…)

 I don't think we should update other occurrences of hton functions in
 this ticket.  Maybe we should discuss the policy in the next call and
 cleanup the code based on its result.

 Other comments on the revised branch:

 '''changelog'''

 Do we need it?

 '''about the constants'''

 - isc::util doesn't seem to be an appropriate place to define them.
   "CC" is specific to the cc module, while util is generally expected
   to be (kitchensink) placeholder for things that don't belong to
   anything else.  It's as awkward as seeing e.g. b10-auth log message
   IDs are defined in isc::util.

 '''msgq.py'''

 - I'm not so sure if handling partial send failure (there are multiple
   possible recipients and a subset of them fail) as an "OK" case; if
   xfrin sends something to multiple instances of auth and not all of
   them were correctly delivered (but the failing auth is still somehow
   running), the system will be in an inconsistent state.  That said,
   this level of discussion is probably beyond the scope of this ticket
   and should go to the larger msgq revisit work.  So I'm making it
   just as a comment, not a request for changing the code.

 '''msgq_test.py'''

 - I can't parse this sentence (the one in the parentheses):
 {{{
         be mostly independant (eg. if the recipient is missing, it
         shouldn't matter why to the handling of the reply header). If
 }}}
   especially around "...why to the...".
 - shouldn't fail_send_prepared_msg return `False` explicitly?
 - the repeated similar patterns in test_undeliverable_errors seem to
   suggest the need for unification, but I'd leave it to you.

 '''session.cc'''

 - unrelated, but noticed: the return type of group_sendmsg seems to be
   wrong: it returns a `long int` value but the type is `int`; often
   the latter is small in size and would cause overflow.

 '''session_unittests.cc'''

 - this is confusing:
 {{{#!cpp
     virtual void sendmsg(ConstElementPtr msg) {
         sendmsg(msg, ConstElementPtr(new isc::data::NullElement));
     }
     virtual void sendmsg(ConstElementPtr env, ConstElementPtr msg) {
         sent_messages_.push_back(SentMessage(env, msg));
     }
 }}}
   in that the `msg` param of the first version is passed to the second
   as the `env` param (and there's another `msg` param in the second
   version).  On looking into it further, I found the confusion was
   actually derived from the base class interface.  I suggest changing
   the param name for the uni-arg version from `msg` to `env` in
   session.h and session.cc, and update the test code accordingly.

 '''session.py'''

 - I'd clarify "if it's set to True and there's no recipient" here:
 {{{
         - want_answer If an answer is requested. If there's no recipient,
           the message queue would send an error message instead of the
           answer.
 }}}

 '''Makefile.am (several)'''

 - In my understanding you don't have to list BUILD_SOURCES things
   in CLEANFILES explicitly.

 '''pythonize_constants.py'''

 - I'd like to see some overview comments (or pydoc) on the purpose of
   this script and what it generally does.
 - not a big deal, but I don't see the point of defining `die`.  It's
   just a two-line function, only used in the `if` block below it.
   Same for const2hdr.py.
 - probably it's better to use 'cc' suffix instead of 'cpp' per our
   file name convention.  same for const2hdr.py.
 {{{#!cpp
     die("Usage: python3 ./pythonize_constants.py input.cpp output.py")
 }}}

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