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 <jch@xxxxxxx> 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
