Hi, all,

One additional transport suggestion:

- it would be useful to include recommended configurations for TCP 
connections. Given these are multi-byte request/response exchanges, 
Nagle should be disabled, e.g.

Joe

On 2/13/2012 2:00 PM, Joe Touch wrote:
> Hi, all,
>
> I've reviewed this document as part of the transport area directorate's
> ongoing effort to review key IETF documents. These comments were written
> primarily for the transport area directors, but are copied to the
> document's authors for their information and to allow them to address
> any issues raised. The authors should consider this review together with
> any other last-call comments they receive. Please always CC
> tsv-dir@ietf.org if you reply to or forward this review.
>
> This request was received Feb. 2, 2012.
>
> This document describes an extension to DHCPv4 for the bulk query and
> transfer of current lease status over TCP.
>
> The following transport issues were noted, and are significant:
>
> UPDATES- The document updates DHCP with support for TCP, and as such
> this document seems like it should UPDATE RFC2131
>
> PORT USE- Although DHCPv4 has an assigned TCP port, this document should
> clearly indicate that there is no other specified use of that port other
> than the bulk lease query described in this document
>
> It should further explain why no other existing DHCP exchanges are not
> valid on the TCP port.
>
> CONNECTION MANAGEMENT- The document describes the use of TCP connections
> for bulk transfer, but needs to be more specific about which side (relay
> client or server) closes the connection, under what circumstances, and
> with what mechanism (e.g., graceful CLOSE vs. ABORT, as per RFC 793)
>
> sec 7.3 indicates some conditions for terminating connections; this
> section should indicate which side performs this, and by what method
> (CLOSE, ABORT)
>
> this sec also allows connections to stay open after all pending
> transactions are complete (MAY); the rationale for this should be given,
> or the connection MUST be closed.
>
> the same issue applies to sec 7.8 and 8 throughout; sec 8.5 is
> particularly problematic on this issue because it discusses aborting a
> request using in-band data, which may not be available if the connection
> is closed using ABORT. the state of other pending connection shsould be
> discussed too.
>
> TIMEOUTS- Sec 6.3 defines a timeout for the TCP connection; is this
> intended to supercede any TCP timeout? or is it intended to be the min
> of the TCP timeout and the specified one?
>
> This section should more carefully explain behavior when a connection is
> dropped and the reason - e.g., timeout, abort, close.
>
> INTERLEAVING- sec 7.7 says that the server MUST interleave replies;
> there doesn't seem a valid reason for this requirement. clearly the
> receiver MUST tolerate interleaved replies. having the server interleave
> replies is relevant only if each request/reply has its own timeout --
> which should be overridden if there is another response in progress
> anyway. This issue should be more clearly explained and motivated.
>
> There were some other issues noted in this document. These comments
> appear below, and although not focused on transport issues, they
> represent significant issues that IMO should be addressed as well.
>
> NOTE - regarding some terminology issues, I did not survey current DHCP
> RFCs for consistency, but IMO these terms should be corrected even if
> they are then inconsistent with existing specs.
>
> Joe
>
> -----------
>
> Major non-transport issues:
>
> - In many places the doc allows inaction to substitute for either
> positive or negative confirmation. IMO, this invites implementation
> errors, and should be avoided. E.g., status return codes, data source
> option missing, query-for-all-configured-IPs, etc.
>
> - the data source option reserved codes need more detailed
> specification. if these are intended for future use, then they MUST be
> ignored by the receiver (at least). if they are to mean anything at all,
> at least one bit (typically all of them) MUST be set to zero by the
> transmitter for implementations that do not support any of the component
> fields.
>
> further, the length of this option MUST be 1
>
> - The protocol supports bulk transfer that is not data driven. This
> could represent a security vulnerability by exposing information that
> may not be on the data path (and thus already accessible) to a relay
> agent. This should be discussed in the security considerations section.
>
> - Integer quantities should be referred to as "unsigned 32-bit integers"
> throughout.
>
> - "VPN" is used throughout to refer to "private" addresses (RFC 1918); a
> VPN is not just private addressing.
>
> - (this is a nit with all IDs, FWIW) SHOULD and SHOULD NOT are used
> throughout without context. Any time SHOULD is used instead of MUST (or
> SHOULD NOT rather than MUST NOT), it is useful to explain a viable
> exception. If no such exception exists, the rationale for selecting
> SHOULD over MUST should be included.
>
> - It would be useful to explain why STATUS-CODE strings MUST NOT be
> null-terminated; is that a protocol violation, or are you just saying
> that NULLs are valid in these strings? the description should be clear
> that the string field describes the string length without any
> termination characters.
>
> - start-time-of-state is expressed as an offset from base-time; this
> appears to be the only time indicated as an offset rather than as an
> absolute. That inconsistency invites implementation errors; IMO, this
> should be absolute too.
>
> - option lengths: throughout, the doc refers to option lengths as being
> "an octet"; they are *indicated* in one octet. Some are constant (e.g.,
> DCHP-STATE), some allow the contents of the octet to vary. Again, this
> is an *unsigned integer* octet.
>
> - some of the information provided (in DHCP-STATE) goes beyond that of
> DHCP. It would be useful to motivate the need for this information in a
> bulk query, and why it is not similarly available for nodes using UDP
> (e.g., as an extension to DHCPv4, not just to the bulk transfer
> command). again, absence of state information should not indicate state.
> State should always be expressed explicitly. these states are further
> meaningless without a state diagram explaining them. if such a diagram
> is not possible (as noted at end of sec 6.2.7), then the states are
> irrelevant and the option should not be included.
>
> - in sec 6.2.9 the term "not allowed" should be explained - are they
> reserved for future use and thus ignored? or are they "not allowed" - in
> which case the doc should indicate handling if they appear.
>
> - sec 7.4 states that the clock skew of zero is desired; this assumes
> E2E delays under 1sec. An explanation of why this is desired should be
> given, as well as the consequences of it not.
>
> Other non-transport issues:
>
> - The document includes definitions and references to irrelevant
> deployment and implementation issues, notably DSLAMs, concentrators, and
> access concentrators. These should not appear formally; they should be
> used only to usefully illustrate currently intended uses.
>
> - The doc refers to "real-time", which could imply requirements not
> supported. This should be replaced with "rapid".
>
> - "absolute time" *indicates* (rather than contains) the number of
> seconds...
>
> - "third party agent" should be explained, i.e., neither DHCP client nor
> DHCP server.
>
> - "downstream" and "upstream" should be defined as both away from the
> server *and towards the client* ("away from the server" has two
> directions).
>
> - sec 3 should refer to relays, and returning the entire set or
> individual bindings; there is no reason to explain the goals in terms of
> access concentrators.
>
> - sec 3.2 appears to provide contradictory advice - caching is required,
> but should be avoided? it would be useful to resolve this inconsistency.
>
> - sec 3.3 refers to 'fast path', but this term doesn't make sense in
> this context because fast-path is a forwarding issue. it would be useful
> to explain what you mean, and pick a different term
>
> ----
>
>
>