Re: [quicwg/base-drafts] QPACK [editorial] Update text on instructions and representations. (#2941)

Mike Bishop <notifications@github.com> Wed, 14 August 2019 18:57 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C8FC5120DC1 for <quic-issues@ietfa.amsl.com>; Wed, 14 Aug 2019 11:57:54 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -8
X-Spam-Level:
X-Spam-Status: No, score=-8 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HR3gzewRbIRc for <quic-issues@ietfa.amsl.com>; Wed, 14 Aug 2019 11:57:52 -0700 (PDT)
Received: from out-23.smtp.github.com (out-23.smtp.github.com [192.30.252.206]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 1B5E0120DBE for <quic-issues@ietf.org>; Wed, 14 Aug 2019 11:57:52 -0700 (PDT)
Date: Wed, 14 Aug 2019 11:57:51 -0700
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1565809071; bh=YIF91IUlzzGs1hAs7I7O2If/FRxh9362iIcO6YZ8fEk=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=kPB8SB+GV3GXQfAU+iB23P9uUrVA6FE8TYyp1T9eoGSc6BZgl2FqjJ7qRxJxtOXJx m7JROQMyGwjUzPcyr7DrAOc4MVMw2m1ad/qcHrQyN04M7Zn78dQNOmAoD1Iz5Wt0Gj njEimYWlYzZqe44bYuP9094m24iIKs+FvVoicFJ0=
From: Mike Bishop <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+AFTOJK2UQZH7ALQE4R3M5N53MGGC7EVBNHHBYU5PJU@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/2941/review/275081269@github.com>
In-Reply-To: <quicwg/base-drafts/pull/2941@github.com>
References: <quicwg/base-drafts/pull/2941@github.com>
Subject: Re: [quicwg/base-drafts] QPACK [editorial] Update text on instructions and representations. (#2941)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5d5459af37927_5ff13fa1c62cd95c310446"; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: MikeBishop
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/A3SeOL75iXAF4BmJuSOM0Ffa78w>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 14 Aug 2019 18:57:55 -0000

MikeBishop approved this pull request.

Mostly good improvements; a few things to fix up further.

> @@ -821,10 +828,13 @@ connection error of type `HTTP_QPACK_DECODER_STREAM_ERROR`.
 
 ## Header Block Representations
 
-Header blocks contain compressed representations of header lists and are carried
-in frames on streams defined by the enclosing protocol.  These representations
-reference the static table, or dynamic table in a particular state without
-modifying it.
+A header block consists of a prefix and a possibly empty sequence of
+representations defined in this section.  Each representation corresponds to a
+single header field.  These representations can reference the static table, or
+the dynamic table in a particular state without modifying it.

The difference from HPACK seems worth noting, but perhaps the sentence could be reworded to flow better.  How about something like "These representations reference the static table and the dynamic table in a particular state, but do not modify the state of the dynamic table."

>  The following bit, 'N', indicates whether an intermediary is permitted to add
 this header to the dynamic header table on subsequent hops. When the 'N' bit is
-set, the encoded header MUST always be encoded with a literal representation. In
-particular, when a peer sends a header field that it received represented as a

I see what you mean about it being a restatement, but it's a ramification of "MUST always" that isn't immediately obvious.  If it's not stated, people will mess this up, I guarantee.  (They do anyway, because not all HPACK libraries tell you whether this bit was set on the way in.)

It's not my favorite feature of HPACK, but let's either keep the clarification or remove the requirement entirely.  If you want to discuss the latter, I would suggest doing so in a separate issue and PR.

>  used to locate the table entry for the header name.  When S=1, the number
 represents the static table index; when S=0, the number is the relative index of
-the entry in the dynamic table.
+the entry in the dynamic table.  Only the header field name stored in the static
+or dynamic table is used. Any header field value MUST be ignored.

Or perhaps make an affirmative statement instead: "Only the header field name is taken from the static or dynamic table entry; the header field value is encoded as a string literal following the index."  There's currently no text that I see referring to the value portion of the instruction.

>  
 
 ### Literal Header Field With Post-Base Name Reference
 
 If the name entry is in the dynamic table with an absolute index greater than or
-equal to the Base, the representation starts with the '0000' four-bit
-pattern. The fifth bit is the 'N' bit as described in
-{{literal-name-reference}}.  Finally, the header field name is represented using
-the post-base index of that entry (see {{post-base}}) encoded as an integer with
-a 3-bit prefix.
+equal to the Base, the representation starts with the '0000' four-bit pattern.
+The fifth bit is the 'N' bit as described in {{literal-name-reference}}.
+Finally, the header field name is represented using the post-base index of that
+entry (see {{post-base}}) encoded as an integer with a 3-bit prefix (see
+{{prefixed-integers}}).

Likewise, no mention here of the string literal for the value.  (Pre-existing problem, not one you introduced.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2941#pullrequestreview-275081269