I have reviewed this document in preparation for IETF last call. Before
proceeding to last call, I have some comments and questions that I’d like to
discuss. I’ve also included a list of nits that should be resolved together
with any last call comments.
== Substantive comments and questions ==
= General
I think this document should be listed as updating RFC 6940 in the document
header.
The document contains a disclaimer for pre-RFC5378 work, which I assume is in
error. If using xml2rfc to generate the text, using the most up-to-date version
of xml2rfc to create the next version should fix this.
= Section 4.3:
"There have been proposals that RouteQuery and a series of Fetch
requests can be used to replace the PathTrack mechanism, but in the
presence of churn such an operation would not, strictly speaking,
provide identical results, as the path may change between RouteQuery
and Fetch operations. (although obviously the path could change
between steps of PathTrack as well)."
This text is confusing. It doesn’t explain why PathTrack is being defined
rather than peers using RouteQuery plus Fetch. Furthermore, the document does
not explain how peers are supposed to interpret the results of the PathTrack
method in the event that the route does change in between steps. This seems
like a substantial omission.
In general, this document provides very little information about how peers are
expected to interpret and use the diagnostic information. I understand that
peers can do whatever they want with it, but in several places the fact that
this is not explained makes the reasoning behind the protocol design decisions
opaque. I would suggest providing a little more context, at the very least by
moving the examples into the body of the draft somewhere.
= Section 4.4:
I’m a little confused about the values given for the error codes. Do you want
IANA to chose these specific values? Or are they just there as placeholders?
The customary way of doing placeholders would be as follows:
Code Value Error Code Name
[TBD1] Underlay Destination Unreachable
[TBD2] Underlay Time exceeded
[TBD3] Message Expired
[TBD4] Upstream Misrouting
[TBD5] Loop detected
[TBD6] TTL hops exceeded
These placeholders would then get used throughout the document.
This section also says:
"In addition, this document introduces several types of error information in
the error_info field in the case of Code 0x65. ... Here are some examples for
the error info. ... The error_info field values of the Code 0x66 to 0x70 are to
be application specific and defined by the particular overlay."
I’m not sure what is meant by all of this text. As defined in RFC 6940,
error_info is an optional implementation-specific byte string. If you are
intending to require implementations to include specific text in the error_info
for the first error code, then you need to explain under what conditions the
specific text needs to be included for each string. And the text about the
other codes seems redundant with how error_info is already defined in RFC 6940.
= Section 5.2:
"ext_length : the length of the returned DiagnosticInfo information
in bytes. If the value is greater than or equal to 1, then some
extended diagnostic information is requested."
This is defining the diagnostic response — why would the extension be
requesting something? And if it is possible to use an extension in that way,
the expected behavior from the initiator when it receives such an extension
needs to be described, or at least it needs to be noted that a diagnostic
response could provoke a request back to the initiator.
Also, in the struct the diagnostic info is called “diagnostic_info_contents”
but in the text it is called “diagnostic_information.” These should be the same
I think.
= Section 5.3:
I’m concerned about the way STATUS_INFO is defined. First, why are there 16
different levels of congestion status? What are they supposed to signify? If
there is no standardized way of measuring congestion defined, how is a node
supposed to interpret STATUS_INFO received from different peers? That is,
couldn't one peer’s level 7 actually mean it is less congested than another
peer’s level 3, and won’t that make the information somewhat meaningless to the
initiator? Maybe this would be clearer if you could explain what you expect an
initiator to do with the information about 16 different levels of congestion.
SOFTWARE_VERSION doesn’t really seem like diagnostic information. While all of
the other fields may be constantly changing and therefore it would make sense
to inquire about them in a diagnostic fashion, SOFTWARE_VERSION seems more like
data that nodes could register or exchange when they join the overlay. Why is
it included as a diagnostic?
Also, doesn’t the SOFTWARE_VERSION type need a length or a specified delimiter
that indicates the end of the string? Otherwise, how is the recipient supposed
to know when to stop parsing it?
The definitions of EWMA_BYTES_SENT and EWMA_BYTES_RCVD seem problematic.
sent = alpha x sent_present + (1 - alpha) x sent
rcvd = alpha x rcvd_present + (1 - alpha) x rcvd
As written these equations are not right because sent/rcvd appear on both
sides. It would be clearer to use last_sent and last_rcvd or some such on the
right-hand side of these equations. But this begs some bigger questions:
- Does this place a requirement on all nodes implementing this specification to
have to calculate these values every 5 seconds?
- How are the values calculated the first time?
- How was the value of 5 seconds chosen?
Finally, I have trouble understanding why only some of the types are subject to
access control as described in Section 7. It seems to me that in some
situations all of the fields defined here could be considered sensitive by
certain nodes or on certain networks. What is the justification for only making
some of them subject to access control?
= Section 6.1:
“The destination field MUST be set to the desired destination, which MAY be
either a NodeId or ResourceID but SHOULD NOT be the broadcast NodeID."
What is the expected behavior if a Ping or a PathTrack does get sent to the
broadcast NodeID? Or should these requirements really be MUST NOTs?
= Section 6.4:
"However, for a single hop
measurement, the traditional measurement methods MUST be used instead
of the overlay layer diagnostics methods."
What is meant by the traditional measurement methods?
= Section 9:
Please only use either RFC-XXXX or RFC-AAAA but not both as placeholders.
Please also use [TBDX] for values you expect to be allocated by IANA.
== Nits ==
= General
The document seems to repeatedly explain that it is doing things “as specified
in RFC6940.” I don’t think this necessary in many cases. It’s worth doing a
pass through and removing the places where it’s obvious that this spec is
building on RFC 6940.
= Section 4.1:
OLD
The extensions strictly follow RELOAD
specification on the messages routing, transport, NAT traversal etc.
NEW
The extensions strictly follow how RELOAD specifies message routing, transport,
NAT traversal, and other RELOAD protocol features.
= Section 4.2.1:
s/as specified in Table 4 and specified in the RELOAD/as specified in Table 4/
s/new requests of the the/new requests of the/
= Section 4.3:
OLD
We define a simple PathTrack method for retrieving diagnostic
information iteratively. The mechanism defined in this document
follows the RELOAD specification, the new request and response
message use the message format specified in RELOAD messages. Please
refer to the RELOAD [RFC6940] for details of the protocol.
The operation of this request is shown below in Figure 2.
NEW
We define a simple PathTrack method for retrieving diagnostic
information iteratively. The operation of this request is shown below in
Figure 2.
= Section 4.3.1.2:
s/after that/after receiving a PathTrackAns where the next_hop node ID equals
the responding node ID/
= Section 4.3.2.1:
s/PathTrack Response/PathTrack response/
= Section 4.4:
s/code. and we define/code. We define/
= Section 5:
s/Path_track methods/PathTrack methods/
= Section 5.1:
s/Note that is NOT/Note that it is not/
= Section 5.2:
s/Note that is NOT/Note that it is not/
s/consists one or more/consists of one or more/
= Section 6.1:
s/`MAY/MAY/
= Section 6.2:
s/return the response the initiator node/return the response to the initiator
node/
s/The peer SHOULD/The intermediate peer SHOULD/
_______________________________________________
P2PSIP mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/p2psip
