Hi Charlie,
Thanks for looking over the changes.
Comments in-line:
On Mon, Nov 23, 2015 at 2:02 PM, Charlie Perkins <
charles.perkins@xxxxxxxxxxxxx> wrote:
Hello folks,were identified in the previous diff. However there were some small changes
I have a few comments on draft 13c, but based only on the highlighted
sections from the Diff file. That way of commenting has limitations that
result when whole sections of text are moved. It then becomes difficult
to compare those sections for changes.
Which is why I chose to move the section, after I made the changes which
Plus I didn't compare draft 13c against draft 12. I fear that will beThe Changes section is not gone, it was just unnecessary for it to contain
pretty difficult. I miss the "Changes" section, which had been kept up
to date in previous revisions.
different metric type may exist in the Local Route Set, but theI dont interpret this text in the same way. "which of these routes to
decision of which of these routes to install in the Routing
Information Base to use for forwarding is outside the scope of
AODVv2.
This is written under the presumption that the RIB cannot contain
multiple routes for the same destination. However, I do not think
we should presume that. It is perfectly reasonable to have a RIB
that includes the metric type as a selector.
MAX_METRIC[MetricType].Of course its possible to violate - when you add incoming metric to the
Somehow this loses the perhaps important information that
typically MAX_METRIC would be chosen to be the maximum
value that is representable. Mandating something that is not
possible to violate is like saying the speed of light MUST NOT
exceed some constant value usually denoted as 'c'.
If such an external process signals that the link is bidirectional,
the value of Neighbor.State MAY be set to Confirmed.
The previous formulation was quite a bit more readable and, thus
in my opinion, better. Perhaps for the most pedantic reviewers,
we could insert a definition something like:
mark a neighbor as Blacklisted
set Neighbor.State to the value Blacklisted
and go back to more readable language.
If one relies on the actual definition of "SHOULD" and "MUST", thenSo we dont have to report when routes are broken? These are active routes,
I think the changes in section 6.9.2 are ill-advised.
When LocalRoute.State changes from Active to Invalid as a result of a
broken link or a received Route Error (RERR) message, other AODVv2
routers MUST be informed by sending an RERR message containing
details of the invalidated route.
The MUST should be a SHOULD. This is because (a) interoperability is
not affected and (b) if the system designer has a better mechanism to
notify for errors, we should not penalize that design and (c) for some
systems, routes may only be used once or only upon rare occasions.
In section 7.4.2. RERR Reception:
1. Update the Neighbor Table according to Section 6.3
It's probably a good idea. Could you describe the expected improvement?
------------------John made a very valid point on this topic in his review on November 4th,
Regarding the deletion of MTU-related mandates:
This is a bad idea. Even TCP is built to respect MTU considerations. It
is
especially a bad idea to avoid mentioning MTU when specifying RERR.
If necessary, I could find quite a few examples in other RFCs where MTU
dictates certain size restrictions.
I'm pretty sure the RFC 5444 authors don't want to be saddled with
creating multiple RERR messages when AODV makes one that is too long.
Fragmentation has its own pitfalls, well known in the Internet.
Please reconsider this. Were there complaints about it??
Regards,In summary, I will remove the comment about updating the neighbor table for
Charlie P.
On 11/10/2015 12:52 AM, Victoria Mercieca wrote:
Just in case it got lost in the reply to the previous thread...
V.
---------- Forwarded message ----------
From: Victoria Mercieca <vmercieca0@xxxxxxxxx>
Date: Sun, Nov 8, 2015 at 2:37 PM
Subject: Re: [aodvv2-discuss] Review of draft 13a (October 25 edition)
To: "aodvv2-discuss@xxxxxxxxxxxxx" <aodvv2-discuss@xxxxxxxxxxxxx>
Hi John (and Charlie),
Thanks for all the feedback, I've been busy updating. Attached a diff and
draft 13c, and comments in line for important stuff:
On Wed, Nov 4, 2015 at 2:39 AM, John Dowdell < <john.dowdell486@xxxxxxxxx>
john.dowdell486@xxxxxxxxx> wrote:
Hi allFor consistency, I changed everything to "control message" rather than
So here’s my review of -13a. I’m aware that there is a -13b but since I’m
halfway through -13a it makes sense to carry on. Apologies if I’m raking up
stuff that’s already been addressed.
Section 1. Overview
I’d like to see an extra paragraph between paras 2 and 3 that explains
some more about AODVv2 sending messages that are converted into packets by
the RFC5444 parser. It would make the existing paragraph 3 a bit clearer.
Maybe move the last but one paragraph starting "AODVv2 control plane
messages use the …” to between paras 2 and 3.
The paragraph starting "AODVv2 uses sequence numbers to identify …” is
not compatible with the text at section 4.4 which suggests that loop
freedom is based purely on sequence numbers “As a consequence, loop freedom
is assured”. I think that last sentence should be deleted from section 4.4,
or at least modified to state that sequence numbers form only part of the
loop freedom calculation. While we’re here, the sentence in section 4.4 is
also incompatible with section 5 bullet 5 “A function to analyse routes …”
Last paragraph: I think the word “plane" should be inserted between
“control” and “messages” (to read “control plane messages” since that makes
it consistent with the last but one paragraph (the one I asked to be moved
above).
"control plane message".
Section 2. Terminology
unicast packet, but we dont know if the link to the neighbour exists, so we
The definition of Neighbor says "Neighbors exchange routing information
and attempt to verify bidirectionality of the link to a neighbor before
installing a route via that neighbor into the Local Route Set.” It doesn’t
say that if the bidirectionally test fails, we do not install a route via
that neighbor. Possibly just remove the words “attempt to”.
The definition of TargSeqNum says "An AODVv2 Data Element used in a Route
Reply message, containing the sequence number of the AODVv2 router which
originated the Route Reply.” could be reworded to say "An AODVv2 Data
Element used in a Route Reply message, containing a sequence number
generated by the AODVv2 router which originated the Route Reply.”
Section 3: Applicability Statement
I believe the opening sentence should be expanded to briefly illustrate
the differences between AODVv2 as a reactive protocol and the proactive
protocols.
Paragraph 5: “Since the route discovery process typically results …”. I
would suggest replacing “typically results in a route being established”
with “requires a route to be established”
Para 8: I recall the question being asked before (probably by Thomas
Clausen) about whether a single router interface can support multiple IP
addresses. If we do, please say, and if not, please also say.
Section 4.3 Neighbor Table
Definition of Neighbor.ResetTime reads "When the State is Blacklisted,
this time indicates the point at which the State reverts to Unknown. By
default this value is …”. Should this read "When the *value of
Neighbor.State i*s Blacklisted, this time indicates the point at which
the *value of Neighbor.State *reverts to Unknown. By default this value
is …”
Section 4.4 Sequence Numbers
Para 3, last sentence, reads "The value zero (0) is reserved to indicate
that the sequence number for an address is unknown.”. Should this read "The
value zero (0) is reserved to indicate that the sequence number for *a
message* is unknown.”?
Para 4: Should “An AODVV2 router can only attach …” read “An AODVv2
router *MUST* attach …”?
Para 5: "As a consequence, loop freedom is assured.”. This implies loop
freedom is a function of sequence number only, but this could well be an
old piece of text. My comment to section 1 applies here.
Section 4.5 Multicast Route Message Table (MRMT). This and section 4.6
could really use some introductory words on the uses of the MRMT, Local
Route Set and the RIB (or other forwarding table belonging to the operating
system), just to set the context for what is to be discussed. This table
also needs to be mentioned in the introductory paragraph of section 6.
RREP: It would be worth explaining when RREPs would be multicast, as it
is not obvious (or wasn’t to me anyway).
we changed this not too long ago - based on the fact that if we create a
dont have a confirmed route to the neighbour itself (maybe also had
something to do with not being able to assume that routers were on the same
subnet) so if we hand a unicast packet down to the forwarding process, it
might not be able to send it if it doesnt have a route, because we havent
confirmed bidirectionality... i can remember being quite confused but the
solution we went for was to multicast the RREP in this case.
All through this section: would be worth tightening up on the name of the
parameter when mentioned (e.g. RteMsg.OrigPrefixLen: The prefix length
associated with *RteMsg*.OrigAddr)
RteMsg.OrigSeqNum: “The sequence number … if present …”. Why would the
sequence number not be present?
RteMsg.RemoveTime: The time .. MUST be removed …”. Removed from where?
Also there are two event times mentioned in this definition, which takes
priority, or is it whichever is earlier or later?
Section 4.6 Local Route Set
The definition of Idle … this could use a cross-reference to section 6.4
where interaction with the forwarding plane of the host operating system is
discussed.
not sure on this...but have shifted some text around to hopefully help. We
might need more work on this though...in Local Route Set we mention RIB...
but in forwarding plane interaction section we mention signals which mean
we should add/remove/update routes... feels like it could be neater, they
could tie up better....
out a better route even though they already have a valid one installed. The
The definition of Invalid … second sentence “… therefore it is not to be
installed in the RIB. …”. Surely “therefore it is *t**o be withdrawn*
from the RIB”?
Para starting “Note that multiple entries …” (bottom of page 15), second
sentence “… but may offer improvement …”. So why is that route still
Unconfirmed? Is it really up to the implementor to decide which routes to
confirm and which to not? This sounds like a potential interoperability
issue to me.
Not interoperability... some implementations could use this to figure
unconfirmed route would offer improvement if it the next hop link could be
confirmed bidirectional. i.e. if there happened to be a RREP in future that
this router was going to forward to that address, it could test the better
unconfirmed route, and if the link then got confirmed, any unconfirmed
routes that use that next hop can be confirmed and the poorer route(s) can
be replaced.
Section 5 MetricsIt woudltn attempt discovery if the source IP address was not configured
Bullet 2 “ …AODVv2 cannot store routes that cost more than
MAX_METRIC(MetricType)”. Anything is possible. Surely “AODVv2 *MUST NOT*
store routes …”?
Section 6.1 Initialisation
The sentence at top of page 18 before the bullets reads “During this wait
period, the router can do the following”. I would suggest to clarify to
“During this wait period, the router *is permitted to* do the
following”. I thought about making this a MUST but thought better of it.
Section 6.2 Next Hop Monitoring
Last sentence of bullet 1, “blacklisted" should read “Blacklisted”.
Section 6.3 Neighbour Table Update
Neighbour.IPAddress … where do we find this information, since it isn’t
(according to the table in section 7.1) explicitly in the RREQ message? I
guess it’s the source address of the RFC5444 packet. In any case, how can
we assure that the neighbour IP address that RFC5444 parser uses is the
same as the one that AODVv2 has allocated to the interface the message was
to be sent over?
Last paragraph in this section on page 20; “However, the reset time
allows ..” should perhaps read “However, *Neighbour.ResetTime* allows …”
Section 6.4 Interaction with the Forwarding Plane
Second bullet in the second batch of bullets reads “A route discovery was
not attempted”. Why would the router not attempt the discovery? The
sentence continues “any buffered packets requiring that the route be
discarded” which should perhaps read “*If any packets are buffered, they
MUST be discarded and the source of the packet should be notified that the
destination is unreachable (using an ICMP Destination Unreachable message”.*
as a router client. The forwarding plane shouldnt have to know about which
addresses AODVv2 will perform route discovery for... it would just say "hey
AODVv2 can you get me a route for this packet" and if the source address
wasnt a router client, AODVv2 would say "nah cant do it for that one,
discard any packets you might have been buffering, i'll send a RERR, (but
maybe you might want to send ICMP destination unreachable...)"
Section 6.6 Route Discovery …source."
Second para last sentence should read “The AODVv2 router *concerned* is
referred to as RREQ_Gen”.
Fourth paragraph reads "Determining which packets to discard first when
the buffer is full is a matter of policy at each AODVv2 router. Routers
without sufficient memory available for buffering SHOULD have buffering
disabled. This will affect the latency for launching TCP applications to
new destinations.” So what is the behaviour here when buffer space is
constrained? Should the router discard the packets while seeking to
discover the route, without sending ICMP Destination Unreachable? Whatever
the behaviour is, please advise some text to put here because it’s not
clear.
Added "If packets are not queued, no notification should be sent to the
Section 6.7 Processing Received Route Information
later in section 7. Section 7 was also confusing, because of 2 things:
“All AODVv2 route messages contain a route”. Surely the RREQ message
generated by RREQ_Gen does not include any route information yet?
Section 6.8 Suppressing Redundant …
First para “… and generates unnecessary signalling traffic and
interference”. Interference? How can we be sure, this will depend on the
exact radio configuration used? Suggest to delete the words “and
interference”.
Page 30 “To determine if a received message …”
Point 1 first bullet “If there is none, the message …” should read “If
there is *no entry*, the message …”
Section 6.9.2 Reporting Invalid Routes
There are three conditions when an RERR SHOULD be sent. Are there no
conditions when an RERR MUST be sent?
Think these should be MUSTS... I think this came from copying words from
1) the step afterwards where we say "if we are PktSource, dont regenerate"
...but thats flawed. If we invalidate any of our routes, we probably want
to report that to others! I removed that step.
2) the fact that if the route was Active, we MUST report it, but if it was
Idle, we might report it, if we configured ENABLE_IDLE_IN_RERR. I've
updated to make this clearer.
Section 7.1.2 RREQ Receptionupdated this bit.
Point 1, bullets 1 and 2; where is Remove Time defined? I couldn’t find a
definition.
Good catch. Its ResetTime in the Neighbor Table as Charlie said. I've
Point 7, suggest the words “is redundant” are replaced by “was previouslyits whether one that was similar but better was previously received. I
received”. The word ‘redundant’ is so abused in the UK (mostly by HR
departments in relation to people they can no longer afford to employ) that
the meaning is now unclear.
I agree with Charlie's comment here... its not "previously received"...
think "redundant" is a simpler way of explaining that.
Section 7.2.1 RREP Generationto include it or not.
Point 2 on page 41: what is the basis on which msg_hop_count is included
in the RREP message?
Good question. It's been in the draft forever with no instruction on when
Section 7.2.2 RREP Reception
Step 1; should the blacklisting also be revoked?
yes thats what is happening when we change state to Confirmed instead of
Blacklisted. I've tidied the neighbor table update step for each message
type since the new section "Neighbor Table Update" handles all the details.
Step 7 7; what should be the action if the route to TargAddr does not
match an outstanding route request? Should the RREP be discarded?
no, because we are probably an intermediate router and we need to
regenerate the RREP toward OrigAddr. Also that statement "if it matches an
outstanding RREQ" fits better in the route processing section referenced,
so I moved it.
Step 9; if not, proceed to step 10?have a route to that address, we would send a RERR to whoever sent the
7.3.2 RREP_Ack Reception
It’s obvious to me, but for the sake of the security AD we probably need
to point out that if the RREP_Ack was not expected, the router must not
respond.
7.4.1 RERR Generation
I was just wondering if we should make an exception for ICMP messages
particularly those signalling errors, and not send an RERR in those cases.
We could potentially have a compound situation where the routers are trying
to deliver ICMP failure messages when the user data IP packet has already
triggered an RERR. Or am I chasing my tail here? This occurred to me
because a router is required to discard the IP packet that triggered the
RERR, and got to wondering whether we should send an ICMP message. I’m open
to discussion here.
If theres an ICMP dest unreachable headed for some address, but we dont
ICMP, to tell them we have no route to forward their packet (whereas their
packet was sent to the source of the packet that triggered the destination
unreachable - AODVv2 wouldnt do anything for this address based on these
facts).
Third bullet starting “When a link breaks …”. There is a point about MTUgeneration and regeneration sections, after the statement that AODVv2
being exceeded … we’re sending AODVv2 messages, not RFC5444 packets. How
will AODVv2 know the MTU is being exceeded? In fact how do we know the MTU
at all, since AODVv2 has not asked to be notified about it by the
forwarding plane in section 6.4. Anyhow is that not the RFC5444 parsers
problem (or maybe even the SMF parser if that is being used as well)?
Perhaps we should convert that text into an advisory to the implementor.
mentions of MTU were scattered around so i have now put them in RERR
message is used to create a RFC5444 message. I think you must be right, it
would be the RFC5444 parsers problem!!
Sending an RERR in response to an undeliverable IP packet … SHOULD bethe next hop toward the source of the undeliverable packet, in which case
sent unicast to the next hop or *alternatively* MUST be multicast …
which is it please? I can sort of see what is being said but the logic is
not clear.
I imagine there may be a case where the router might not have a route to
it should multicast the RERR.
Section 7.4.2 RERR Reception
Typo … Step 1 bullet 1 .. ignore this RREP … should read ignore this
*RERR*.
Good catch.
I believe we need to use stronger language in the sentence “If any of the
above are false …”, I propose "If any of the above are false, the
LocalRoute does not need to be made Invalid and the unreachable address
does not need to be advertised in a regenerated RERR.” be changed to "If
any of the above are false, the LocalRoute *MUST NOT* be made Invalid
and the unreachable address *MUST NOT* be advertised in a regenerated
RERR.”
There are conflicting directives all over the second half of this
section. If we have routes matching those in the RERR, SHOULD or MUST an
RERR be regenerated? There is no defining logic here. The bullets in step 2
sometimes say we SHOULD, the directive at step 4 is to do it anyway. There
is no clear decision tree. I propose that we MUST regenerate an RERR in all
cases if the RERR passes the validity test in the first half of this
section.
as mentioned earlier, have updated this.
Section 7.4.3 RERR Regeneration
Step 5 … again we need to advise the implementor that an RERR with lots
of addresses could break an MTU and so there needs to be a check somewhere
when the packet is to be sent to the interface. AODVv2 is not in charge of
forming the packet.
Again … SHOULD be unicast or MUST be sent multicast? Which is it please?
Suggest something like "if the message cannot be unicast it MUST be sent
multicast”.
8. RFC5444 Representation.
Hold up. Control plane message SHOULD use RFC5444? I thought that was the
whole point of AODVv2. Surely MUST?
True, its MUST.
I haven’t checked the mapping to RFC5444 TLVs because my head hurts, but
it looks beautiful.
Section 11.2 Protocol Constants / Section 11.6 MetricType Allocation
MAX_METRIC[MetricType] … just a note for the future that DLEP had a
discussion on the field length of a metric. Might as well recommend that
AODVv2 metric fields are compatible with DLEP.
Any suggested text?
Section 13 Security Considerationson a printed copy?
There is a theme of reliance on ICV to show the message has integrity.
That’s ok by itself, but it should also be pointed out that the messages
must be robustly processed according to the steps in section 7 (which must
itself be robustly specified). If there are holes in our specifications, or
if there are implementation holes, or chances for common attack vectors
(e.g. sending a huge RERR causing a buffer overflow), then that’s a
problem, particularly for the constrained memory devices we have discussed
in the past.
Appendix C.
My page 74 is blank for some reason.
looked ok on my screen, page 74 definitely had contents...maybe an issue
Similarly there are some text flow problems in this section, perhapstries to keep it on one page, and often puts it on the next page, however
where code blocks are included, e.g. C.2.1.3, C.2.2.
this is a pandoc quirk. By marking the pseudo-code as a code block it
the heading is not part of this block so stays on the preceding page.
I’m, not going to check the code, not really my area!
Overall
Quite a fabulous document. Looks a whole lot better than it did a year or
so ago. Well done all.
Cheers :)
CheersRegards,
John
Vicky.
