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
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:10 jinmei]:
 > - Fixing this is easy, but it seems to me to suggest another
 >   background issue of the implementation: we rely on these hardcoded
 >   keywords too much.  As long as we hardcode magic keywords like
 >   "want_reply" in different multiple places, this type of error is not
 >   avoidable.  So, I wonder whether we can use this opportunity to
 >   introduce a more robust solution: introduce a unified source
 >   (auto-parsable plain text or json or whatever) that defines these
 >   keywords and some other constants, and generate C++ and Python files
 >   from it.  For example:

 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?

 > - A different topic: I don't whether there's any documentation about
 >   msgq protocol, but there should be some (if not existent right now,
 >   create it), which explains how "wants_reply" or "want_answer" or
 >   whatever works along with the header format.

 There's the ticket #2671, which should update the existing documentation.
 I'll add a note about the want_answer header there, so it includes it.

 > - what if msgq tries to send a message to a module but it fails with
 >   EPIPE?  should it treat it like msgq thinks there's no recipient?

 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.

 > - 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 probably beyond the scope of this task, but I'm not sure if
 >   this is true:

 I reworded it a little bit and moved.

 > - First, I have some primitive question: do we really need the actual
 >   network I/O setup for these tests?  All we need to do is to confirm
 >   group_sendmsg() sets various header fields as expected depending on
 >   its parameters.  It seems to be possible with much easier setup if
 >   we can accept some small modification to the `Session` class:
 >   specifically, making `sendmsg()` a protected function, inherit a
 >   mock `Session` class from the real one, stealing (only) sendmsg,
 >   and examine the passed parameter to the faked sendmsg from
 >   group_sendmsg() in the test.  This approach has some obvious
 >   drawbacks, but if the alternative is to rely on actual network I/O
 >   with ASIO-level hassles + safety net alarm, I personally think it's
 >   much better.  Below, however, I'll make my comments based on the
 >   current setup and implementation.

 You're right in that. Actually, the method doesn't need to be even
 protected, it can be private. I changed it that way.

 > - s/for ever/forever/?

 It doesn't matter now, but I believe both are correct.

 > - 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…)

 > - combining these two, we could even integrate these two version:

 Yes, it was a relict of when I used both in some temporary version of code
 and forgot to clean up.

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