Subject: Miscellaneous Comments on -14

From: "Simpson, Robby (GE Energy Management)" <[email-masked]>
Date: Mon, 4 Aug 2014 19:34:33 +0000

Here are some miscellaneous comments based on my read of -14.  Please let
me know if there is some other way I should submit (bug tracker, etc.)

Various ASCII art header figures show fields aligned on byte and word
boundaries (e.g., "DATA Frame Payload", "HEADERS Frame Payload", "Setting
Format", "PUSH_PROMISE Payload Format").  The text doesn't mention this at
all.  Is byte and word alignment intended?

The plurality of the phrase "header block fragment" is ambiguous to me and
seems to vary in the text.  Can a HEADERS, PUSH_PROMISE, or CONTINUATION
frame contain "fragments" or "a fragment"?

In 6.6, "PADDED (0x8):  Bit 4 being set indicates that the Pad Length
field is present."  This needs to also indicate that "Padding" may also be
present, dependent on the Pad Length.  Also "Padding: Padding octets"
needs to state that it is only present if PADDING flag is set and Pad
Length > 0.

GOAWAY frame - does the "Additional Debug Data" count toward flow control?
 I think not, but I could see this field growing large.  I understand we
may not wish to block GOAWAY frames, but there may be a hole here with an
unbounded size.  (HEADERS, as a counter-example, at least has a limited
size in SETTINGS).  Is it OK just to rely on MAX_FRAME_SIZE?

9.2 states "Implementations of HTTP/2 MUST support TLS 1.2 [TLS12] for
HTTP/2 over TLS." then "An implementation of HTTP/2 over TLS MUST use TLS
1.2 or higher with the restrictions on feature set and cipher suite
described in this section." So which is it?  == 1.2 or >= 1.2

In 6.5.2, SETTINGS_MAX_FRAME_SIZE initial value is defined "The initial
value is 2^14 (16,384) octets."  In 11.3 its 65536.

Nits:

End of page 9, beginning of page 10: Need a "For example:" or other
introductory text.

The text for the state "reserved (local)" and "reserved (closed)" could be
made similar.
  "An endpoint MUST NOT send frames other than HEADERS or RST_STREAM in
this state."
  and
  "A PRIORITY frame MAY be received in this state.  Receiving any frames
other than RST_STREAM, or PRIORITY MUST be treated as a connection error
(Section 5.4.1) of type PROTOCOL_ERROR."
  vs.
  "An endpoint MAY send a PRIORITY frame in this state to reprioritize the
reserved stream.  An endpoint MUST NOT send any other type of frame other
than RST_STREAM or PRIORITY."
  and
  "Receiving any other type of frame other than HEADERS or RST_STREAM MUST
be treated as a connection error (Section 5.4.1) of type PROTOCOL_ERROR."

The first and last paragraphs of 5.3 are a bit redundant:
  "A client can assign a priority for a new stream by including
prioritization information in the HEADERS frame (Section 6.2) that opens
the stream.  For an existing stream, the PRIORITY frame (Section 6.3) can
be used to change the priority."
  and
  "Prioritization information can be specified explicitly for streams as
they are created using the HEADERS frame, or changed using the PRIORITY
frame.  Providing prioritization information is optional, so default
values are used if no explicit indicator is provided (Section 5.3.5)."

In 5.4.3, "If the TCP connection is torn down" should be something like
"If the TCP connection is reset or closed".  I'm not sure "torn down" is
really defined.

6.1 and elsewhere: "The DATA frame defines the following flags:"  This
does not clearly indicate where the flags are set.  We likely all know the
field, but the draft does not state it.

The phrase "This field is optional and is only present if the PRIORITY
flag is set" occurs a few times.  Suggest removing "is optional and".

The phrase "A HEADERS frame that is followed by CONTINUATION frames
carries the END_STREAM flag that signals the end of a stream.  A
CONTINUATION frame cannot be used to terminate a stream." from 6.2 is
unclearly worded.

The phrase "This unsigned 31-bit integer identifies the stream the
endpoint intends to start sending frames for." (6.6) would make my grammar
school teacher shudder.  ;-)

In 6.9.1, "For flow control calculations, the 8 byte frame header is not
counted." should be octets.

In 7, "Error codes share a common code space.  Some error codes apply only
to either streams or the entire connection and have no defined semantics
in the other context."  I would suggest re-wording as this could be
misread.

In 8.3, "The HTTP header field mapping works as mostly as defined in
Request Header Fields (Section 8.1.2.3), with a few differences."  Typo?

- Robby

Robby Simpson, PhD
System Architect
GE
Digital Energy

---

From: Martin Thomson <[email-masked]>
Date: Wed, 13 Aug 2014 10:29:40 -0700

I think that I got these.

> all.  Is byte and word alignment intended?

Byte, yes.  Word, no.

> frame contain "fragments" or "a fragment"?

I went through and corrected one instance.  If you can be more
specific, then I can too.

> needs to state that it is only present if PADDING flag is set and Pad

OK.

> GOAWAY frame - does the "Additional Debug Data" count toward flow control?

No.

> size in SETTINGS).  Is it OK just to rely on MAX_FRAME_SIZE?

I think so.  There is plenty of incentive to keep this small already.
Realistically, MSS is likely to be a limiting factor.

> described in this section." So which is it?  == 1.2 or >= 1.2

It's >= 1.2.  Supporting 1.2 is a natural prerequisite of all TLS
versions above 1.2.

> value is 2^14 (16,384) octets."  In 11.3 its 65536.

Someone caught that for me already.

> in the other context."  I would suggest re-wording as this could be

Taking suggestions.

---

From: "Simpson, Robby (GE Energy Management)" <[email-masked]>
Date: Thu, 28 Aug 2014 19:23:27 +0000

>> all.  Is byte and word alignment intended?

So I think I've confused the issue by talking about words.  What I really
mean is that the ASCII art shows fields starting at 32-bit boundaries when
they do not.  For example (from the latest Editors' copy):

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                 Length (24)                   |
+---------------+---------------+---------------+
|   Type (8)    |   Flags (8)   |
+-+-+-----------+---------------+-------------------------------+
|R|                 Stream Identifier (31)                      |
+=+=============================================================+
|                   Frame Payload (0...)                      ...
+---------------------------------------------------------------+

This makes it look like "Type" begins at bit 32.  Whereas I would think
the specification states something like:

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                 Length (24)                   |    Type (8)   |
+---------------+-+-----------------------------+---------------+
|   Flags (8)   |R|                 Stream Identifier (31)
+---------------+-+-------------+===============================+
                                | Frame Payload (0...)        ...
+-------------------------------+===============================+

If you agree, let me know and I can check out an editors' copy and take a
stab at them.

>It's >= 1.2.  Supporting 1.2 is a natural prerequisite of all TLS

I'm not convinced - to me there is a difference between "MUST support 1.2"
vs. "MUST use 1.2 or higher".