Re: [RTG-DIR] Rtgdir telechat review of draft-ietf-babel-rfc6126bis-11

[Yingzhen Qu <[e-mail]>]

Hi Juliusz,

Thanks for your reply, please see my response below starting with [YQ].

Thanks,
Yingzhen

On Fri, Aug 2, 2019 at 1:20 AM Juliusz Chroboczek <[e-mail]> wrote:

> Dear Mr Qu,
>
> Thank you for your review.
>
> > The description about packet trailer is in RFC 7557 section 2.5, but not
> > included in the bis version. Considering this document will obsolete RFC
> 7557,
> > I suppose it should be self-complete, so readers don’t need to go back
> to RFC
> > 7557 for more information.
>
> The packet trailer is described in Section 4.2.
>
> [YQ]: In RFC 7557 section 2.5, there is a clear description about how the
packet trailer length is calculated etc. Personally I feel it's more
straightforward, but if you think section 4.2 is enough, I'm not going to
insist.


> > There are multiple places in the document about TLV/Sub-TLV being
> > “self-terminating”, but I didn’t find what it means. Maybe I’m missing
> > something here?
>
> This is defined in Section 4.4.  I have checked that all uses of
> self-terminating occur after its definition.
>
> [YQ]: in section 4.4, it says self-terminating can be used to determine
the length of the TLV body without using the length field. This is clear,
however what does it look like? how to detect it? maybe I'm missing
something here.


> > Section 1.1
> > “unmanaged and wireless environment”, what does “unmanaged” mean here?
>
> Not being actively managed by a human administrator.  This is a
> non-normative
> section.  Please let me know if I'm using this term badly.
>
> [YQ]: my understanding is unmanaged can mean anything, like no management
at all. I don't think it's a good term, especially put together with
wireless.  maybe try to add a bit more explanation?

> Section 3.2.3
> > The interface Hello seqno is changed to outing multicast hello seqno,
> however
> > there is no description about unicast hello at all.
>
> Unicast Hellos are per-neighbour, so they appear in Section 3.2.4 (Section
> 3.2.3 describes per-interface data structure while 3.2.4 describes
> per-neighbour structures).  Both kinds of Hellos are described in more
> detail in Section 3.4.1.
>
[YQ]: there is nothing wrong with current text. I was thinking of adding
unicast just for easy readability. same for the following three related
questions.

>
> > While I was reading it, I kept thinking what if it’s unicast hello? Is
> > there a unicast hello timer needed?
>
> There is one, see Section 3.2.4, penultimate paragraph.
>
> > From later sections, I realized there are unicast hellos. So I’d suggest
> > add some descriptions to avoid the confusion.
>
> Sections 3.2.4 and 3.4.1.
>
> > Section 3.2.4
> > There is “the expected incoming Unicast Hello sequence number” and “the
> > outgoing Unicast Hello sequence number”, but it was not mentioned in
> section
> > 3.2.3 (related with previous comment).
>
> That's right.  Section 3.2.3 describes per-interface data.  Section 3.2.4
> describes per-neighbour data.  Multicast Hellos are per-interface.
> Unicast Hellos are per-neighbour.
>
> > Section 3.8.1.1
> > After a node receives a route request, if the given prefix doesn’t exist
> in its
> > route table, it MUST send a retraction for that prefix. So my question is
> > whether a node is allowed to send multiple explicit requests for a given
> > prefix?
>
> There's nothing forbidding it.  It may send multiple requests to different
> neighbours (over unicast), or multiple requests on different interfaces
> (over multicast).  There's also nothing forbidding a node from sending
> multiple requests to the same destination, e.g. to compensate for packet
> loss.
>
> (Our implementation experience shows that, at least over 802.11, such
> aggressive behaviour is not useful, it increases noise without having
> a measurable effect on convergence time, hence the SHOULD in Section
> 3.8.2.3 which only requires a single request.  However, further research
> might yield new insights, which is why this document does not explicitly
> disallow such behaviour.)
>
> [YQ] thanks for the explanation. I'm just wondering whether we should
document it somewhere?


> > If so, what happens if both retractions and updates are received?
>
> The procedure described in Section 3.5.4 is run for each received update
> or retraction, which results in the construction of the data structures
> used as input for the precedure described in Section 3.6.
>

[YQ]: got it.


> > 1616    4.4.  Sub-TLV Format
>
> > This section is about Sub-TLV, however the description language in this
> > paragraph is mainly about TLV.
>
> Good catch, thanks.  I'll fix that.
>

[YQ]: same as comments regarding unicast hello, current text is not wrong.

>
> > Section 4.5
> > it says that “an implementation may choose to use a dedicated stateless
> parser
> > to parse the packet trailer”. Will the packet trailer be able to use the
> state
> > parser if there are state there useful or just for implementation
> purpose?
>
> I do not understand this comment.  Please clarify.
>
> [YQ]: My understanding is Babel uses a stateful parser, and TLVs in the
packet trailer are not allowed to modify the state. So the text here is
suggesting to implement a stateless parser for packet trailer. so the
question is: will packet trailer to able to use/read the state? is the
stateless parser suggested just for implementation simplicity?


> > Although there is no packet trailer defined at this moment.
>
> Section 4.2.
>
[YQ]: I meant besides the pad1 and padN, there is no real functional packet
trailer defined at this moment, but it's open for extensions.

>
> > Section 4.5
> > 1674       parsing a TLV MUST update the parser state even if the TLV is
> > 1675       otherwise ignored due to an unknown mandatory sub-TLV.
>
> I believe you may have forgotten to write your comment.
>
> [YQ]: is it correct that the parser state is updated by a TLV even if the
TLV is ignored due to unknown sub-tlv? what about a TLV ignored due to
other reasons?

> Section 4.6.5
> > 1805                 send a new scheduled Hello TLV with the same
> setting of the
> > 1806                 Unicast flag.  If this is 0, then this Hello
> represents an
> > I’d suggest changing the text to “the same setting of flags” instead of
> “the
> > same setting of the Unicast flag”, considering the flags will be
> extended later.
>
> I disagree.  This paragraph is about the multicast/unicast dichotomy, not
> about future flags of an informative nature.
>
> [YQ]: then I misunderstood it. I thought it was meant to copy the entire
flag field.

> Nits:
>
> > 1049       metric) from a neighbour neigh with a link cost value equal
> to cost,
> > 1050       it checks whether it already has a route table entry indexed
> by
> > [neighbour neigh] => [neighbour]
>
> I disagree.  We're defining the variable neigh, which we use in the next
> sequence.
>
> [YQ]: it seems that this is the only place using "neigh", so I thought it
was a typo. I would suggest some clarification.



> Regards,
>
> -- Juliusz Chroboczek
>