[qpack] minor edits #4424

afrind · 2020-12-05T00:30:43Z

No description provided.

ianswett

Some small suggestions that I think might improve consistency, but no strong feelings.

ianswett · 2020-12-05T16:25:56Z

draft-ietf-quic-qpack.md

@@ -1032,7 +1033,7 @@ the Base.
 ~~~~~~~~~~
 {: title="Indexed Field Line"}

-This representation starts with the '1' 1-bit pattern, followed by the 'T' bit
+This representation starts with the '1' one-bit pattern, followed by the 'T' bit
 indicating whether the reference is into the static or dynamic table.  The 6-bit


For consistency?

Suggested change

indicating whether the reference is into the static or dynamic table. The 6-bit

indicating whether the reference is into the static or dynamic table. The six-bit

I should have mentioned this in the summary. This is intentional, because the spec defines an "N-bit prefix" to mean something very specific, and that was consistently used in numeric format. HPACK also had the "one-bit pattern"/"4-bit prefix" as well. It just got out of sync in some of the QPACK additions.

I agree with Ian that it would be nice to have a consistent rule across pattern and prefix about writing number numbers with digits versus words. For example, always write with digits, or always write out in words. I've also seen style guides recommending words for numbers up to ten and digits starting from 11 (just like in this sentence). Well, N is always at most 7, so we should probably always use digits or always use words, but preferably do the same for patterns and prefixes within a single document.

I agree with Alan that consistency with HPACK is also desirable. Now HPACK has 5 occurrences of "N-bit pattern" and 14 occurrences of "N-bit prefix" with specific values for N, and every single one of them uses digits to represent N (just like in this sentence).

In contrast, QPACK before this change has 10 occurrences of N-bit pattern with N in words, 2 occurrences of N-bit pattern with N in digits, 0 occurrences of N-bit prefix with N in words, and 20 occurrences of N-bit prefix with N in digits (only counting occurrences where N is a specific number, not A or N or N-1). This change would make all 12 patterns written in words but leave all 20 prefixes in digits. For consistency, I suggest changing all N-bit patterns to writing N in digits.

@bencebeky : derp, you are absolutely right. I know I looked at HPACK for consistency but in my head saw it was spelled out in some cases -- must be moving too fast. I'll update it to be digits everywhere, which is both self-consistent in this document consistent with HPACK.

ianswett · 2020-12-05T16:26:14Z

draft-ietf-quic-qpack.md

-an integer with a 4-bit prefix; see {{prefixed-integers}}.
+This representation starts with the '0001' four-bit pattern.  This is followed
+by the post-base index ({{post-base}}) of the matching field line, represented
+as an integer with a 4-bit prefix; see {{prefixed-integers}}.


Suggested change

as an integer with a 4-bit prefix; see {{prefixed-integers}}.

as an integer with a four-bit prefix; see {{prefixed-integers}}.

ianswett · 2020-12-05T16:26:35Z

draft-ietf-quic-qpack.md

@@ -1143,7 +1144,7 @@ field name and a field value as string literals.
 ~~~~~~~~~~
 {: title="Literal Field Line With Literal Name"}

-This representation begins with the '001' three-bit pattern.  The fourth bit is
+This representation starts with the '001' three-bit pattern.  The fourth bit is
 the 'N' bit as described in {{literal-name-reference}}.  The name follows,
 represented as a 4-bit prefix string literal, then the value, represented as an


Suggested change

represented as a 4-bit prefix string literal, then the value, represented as an

represented as a four-bit prefix string literal, then the value, represented as an

ianswett · 2020-12-05T16:26:48Z

draft-ietf-quic-qpack.md

@@ -1143,7 +1144,7 @@ field name and a field value as string literals.
 ~~~~~~~~~~
 {: title="Literal Field Line With Literal Name"}

-This representation begins with the '001' three-bit pattern.  The fourth bit is
+This representation starts with the '001' three-bit pattern.  The fourth bit is
 the 'N' bit as described in {{literal-name-reference}}.  The name follows,
 represented as a 4-bit prefix string literal, then the value, represented as an
 8-bit prefix string literal; see {{string-literals}}.


Suggested change

8-bit prefix string literal; see {{string-literals}}.

eight-bit prefix string literal; see {{string-literals}}.

LPardue · 2020-12-07T17:56:04Z

@afrind is this linked to any issue?

afrind · 2020-12-07T18:07:31Z

@LPardue : no actually. I should probably create one I guess? These came from a review from a past colleague with a sharp eye for detail.

LPardue · 2020-12-07T18:12:19Z

For the scope of changes, I think its fine under an editor prerogative. Just wanted to make sure we weren't missing a linkage, alls good.

bencebeky · 2020-12-08T15:48:03Z

draft-ietf-quic-qpack.md

@@ -1235,8 +1235,9 @@ recovered successfully. However, values with low entropy remain vulnerable.
 Attacks of this nature are possible any time that two mutually distrustful
 entities control requests or responses that are placed onto a single HTTP/3
 connection. If the shared QPACK compressor permits one entity to add entries to
-the dynamic table, and the other to access those entries, then the state of the
-table can be learned.
+the dynamic table, and the other to access those entries to encode chosen field


Not sure if including this commit (from PR 4423) in this PR is intentional.

Nope, not intentional, I think I did a git checkout -b from that commit instead of HEAD. But I think if I merge that one first it's not an issue?

bencebeky · 2020-12-08T16:05:07Z

draft-ietf-quic-qpack.md

@@ -1032,7 +1033,7 @@ the Base.
 ~~~~~~~~~~
 {: title="Indexed Field Line"}

-This representation starts with the '1' 1-bit pattern, followed by the 'T' bit
+This representation starts with the '1' one-bit pattern, followed by the 'T' bit
 indicating whether the reference is into the static or dynamic table.  The 6-bit


I agree with Ian that it would be nice to have a consistent rule across pattern and prefix about writing number numbers with digits versus words. For example, always write with digits, or always write out in words. I've also seen style guides recommending words for numbers up to ten and digits starting from 11 (just like in this sentence). Well, N is always at most 7, so we should probably always use digits or always use words, but preferably do the same for patterns and prefixes within a single document.

I agree with Alan that consistency with HPACK is also desirable. Now HPACK has 5 occurrences of "N-bit pattern" and 14 occurrences of "N-bit prefix" with specific values for N, and every single one of them uses digits to represent N (just like in this sentence).

In contrast, QPACK before this change has 10 occurrences of N-bit pattern with N in words, 2 occurrences of N-bit pattern with N in digits, 0 occurrences of N-bit prefix with N in words, and 20 occurrences of N-bit prefix with N in digits (only counting occurrences where N is a specific number, not A or N or N-1). This change would make all 12 patterns written in words but leave all 20 prefixes in digits. For consistency, I suggest changing all N-bit patterns to writing N in digits.

* Replace "0" with 0 when referring to indexes * Make all instructions/representations consistent 'starts with the 'X' <number spelled out>-bit pattern' * Added a line break to help disambiguate 'These streams'

afrind requested a review from MikeBishop as a code owner December 5, 2020 00:30

ianswett requested a review from bencebeky December 5, 2020 01:05

ianswett approved these changes Dec 5, 2020

View reviewed changes

LPardue approved these changes Dec 7, 2020

View reviewed changes

LPardue added -qpack editorial An issue that does not affect the design of the protocol; does not require consensus. labels Dec 7, 2020

bencebeky reviewed Dec 8, 2020

View reviewed changes

afrind added 2 commits December 8, 2020 15:13

[qpack] Minor edits

bbd6776

* Replace "0" with 0 when referring to indexes * Make all instructions/representations consistent 'starts with the 'X' <number spelled out>-bit pattern' * Added a line break to help disambiguate 'These streams'

Replace <spelled-out-number>-bit pattern to N-bit pattern

6e70f5c

afrind force-pushed the qpack-minor-edits branch from c52f802 to 6e70f5c Compare December 8, 2020 23:16

afrind merged commit 51f74ee into master Dec 8, 2020

afrind deleted the qpack-minor-edits branch December 8, 2020 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[qpack] minor edits #4424

[qpack] minor edits #4424

afrind commented Dec 5, 2020

ianswett left a comment

ianswett Dec 5, 2020

afrind Dec 7, 2020

bencebeky Dec 8, 2020

afrind Dec 8, 2020

ianswett Dec 5, 2020

ianswett Dec 5, 2020

ianswett Dec 5, 2020

LPardue commented Dec 7, 2020

afrind commented Dec 7, 2020

LPardue commented Dec 7, 2020

bencebeky Dec 8, 2020

afrind Dec 8, 2020

bencebeky Dec 8, 2020

	indicating whether the reference is into the static or dynamic table. The 6-bit
	indicating whether the reference is into the static or dynamic table. The six-bit

	as an integer with a 4-bit prefix; see {{prefixed-integers}}.
	as an integer with a four-bit prefix; see {{prefixed-integers}}.

	represented as a 4-bit prefix string literal, then the value, represented as an
	represented as a four-bit prefix string literal, then the value, represented as an

	8-bit prefix string literal; see {{string-literals}}.
	eight-bit prefix string literal; see {{string-literals}}.

[qpack] minor edits #4424

[qpack] minor edits #4424

Conversation

afrind commented Dec 5, 2020

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPardue commented Dec 7, 2020

afrind commented Dec 7, 2020

LPardue commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment