Subject: [Gen-art] Gen-ART review of draft-ietf-hybi-permessage-compression

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Tue, 16 Jun 2015 14:47:46 +0000

I am the assigned Gen-ART reviewer for this draft. For background on Gen-ART, please see the FAQ at

<https://urldefense.proofpoint.com/v2/url?u=http-3A__wiki.tools.ietf.org_area_gen_trac_wiki_GenArtfaq&d=AwICAg&c=BFpWQw8bsuKpl1SgiZH64Q&r=I4dzGxR31OcNXCJfQzvlsiLQfucBXRucPvdrphpBsFA&m=hGLCrvRMbwg7aF87DUcj1kzIZxtq-RxIeVZwskYlZUw&s=JAEyywKQRPrtTGWFwQ10cWxvPlWJJMPtFco6XfIaGDg&e= <https://urldefense.proofpoint.com/v2/url?u=http-3A__wiki.tools.ietf.org_area_gen_trac_wiki_GenArtfaq&d=AwICAg&c=BFpWQw8bsuKpl1SgiZH64Q&r=I4dzGxR31OcNXCJfQzvlsiLQfucBXRucPvdrphpBsFA&m=hGLCrvRMbwg7aF87DUcj1kzIZxtq-RxIeVZwskYlZUw&s=JAEyywKQRPrtTGWFwQ10cWxvPlWJJMPtFco6XfIaGDg&e=%20> >.

Please resolve these comments along with any other Last Call comments you may receive.

Document: draft-ietf-hybi-permessage-compression-21

Reviewer: Dan Romascanu

Review Date: 6/16/15

IETF LC End Date: 6/24/15

IESG Telechat date: 7/9/15

Summary: Ready with minor issues

I do not know if this is an issue, but I am asking the question. Some implementations are mentioned in the write-up, but they seem to have been written and deployed based on very old versions of the document - 05 and earlier, dating three years back. I am wondering if this is a problem of the write-up not being updated. If not - have the changes brought to the document since draft-05 impacted the negotiation and algorithms described in the document?

Major issues:

Minor issues:

1.       In Section 5:

   The server must keep the original
   bytes of data that it recently compressed and sent to the client.
   The client must keep the result of decompressing the bytes of data
   that it recently received from the server.

   The two 'must' here seem to deserve capitalization.

2.       In Section 8.2.1:

Note that

   for non-final fragments, the removal of 0x00 0x00 0xff 0xff must not

   be done.

'must not' here seems to deserve capitalization.

Nits/editorial comments:

1.       In Section 4: 'One PMCE extension is defined ...' - the word extension can be dropped, E already stands for extension

2.       In Section 10.2 - It would be good to explain in 'Meaning' the significance of the values 0 and 1 of the RSV1 bit

3.       In section 11:  s/Thank you to the following people/Thanks to the following people/

---

From: Barry Leiba <[email-masked]>
Date: Tue, 16 Jun 2015 16:08:37 +0100

Hi, Dan, and thanks for the review.

> I do not know if this is an issue, but I am asking the question. Some
> implementations are mentioned in the write-up, but they seem to have been
> written and deployed based on very old versions of the document – 05 and
> earlier, dating three years back. I am wondering if this is a problem of the
> write-up not being updated. If not – have the changes brought to the
> document since draft-05 impacted the negotiation and algorithms described in
> the document?

The history here is that the working group sent the document to me at
version -05, and I reviewed it and sent it back for more editorial
work -- I found it very hard to read and often unclear.
Unfortunately, the editorial work took two years and many revisions to
complete, but I think the result is clear and easy to read, and I'm
happy with the result.  But that work did not make technical changes
to the protocol, so implementations of -05 are still implementations
of -22... unless, of course, they were based on any misunderstandings
of an unclear document.  I think the likelihood of that last is low,
because the implementors were involved in the development of the
document.

On the minor issues:
I'm ambivalent about changing the "must"s and "must not" to 2119 key
words.  I think you know that I'm not a fan of overuse of those key
words.  On the other hand, they do apply fine here as key words, and I
have no objection to changing them.  I'll leave that up to the author
and shepherd.

Nits 1 and 2 are correct, and should be fixed; thanks.  For nit 3, I'm
happy to have the acknowledgments section reflect the author's voice.

Barry

---

From: Takeshi Yoshino <[email-masked]>
Date: Tue, 23 Jun 2015 15:52:04 +0900

Thank you for review, Dan.

> because the implementors were involved in the development of the

Here're the non-editorial changes happened since -05:

-10 added hints. This is optional feature. Shouldn't impact
interoperability.

-10 made it mandatory for clients to support client_no_context_takeover (at
this point it was called c2s_no_context_takeover) parameter in an extension
response. This shouldn't introduce any spec issue, I think.

-15 renamed s2c_ to server_ and c2s_ to client_. I think everyone has
followed this change in this 1.5 year.

Changing "must" to "MUST" in Section 5:

>From 2 perspectives, I prefer not to capitalize them:
- (a) These sentences are describing what/how constraints have to be
negotiated for the DEFLATE based compressed communication channel just as
an example.
- (b) These sentences are not explaining requirements that implementors
must follow for interoperability, but just things they have to do to make
DEFLATE based compression/decompression work correctly.

Because of (b), I'd like to keep these "must keep"s uncapitalized. I'm
ambivalent about (a) regarding "MUST" and "may" the following sentences

   In a PMCE negotiation offer, the client MUST inform the server of its
   LZ77 window size so that the server uses an LZ77 window size that is

   LZ77 window size of the client.  In a PMCE negotiation offer, the
   client may inform the maximum LZ77 window size the client can afford

We can consider these to be kind of quotes of conformance requirements.
Actually, we're defining them later in the same doc. So, I'd like to keep
this MUST and change the "may" in the second sentence to "MAY" for
consistency. Does this look good to you?

----

Changing "must not" to "MUST NOT" in Section 8.2.1: Agreed. I'll fix it.

----

Nit 1: Removed the redundant "extension".

Nit 2: Added explanation of the significance of the values 0 and 1 as
follows:

   Meaning
      The message is compressed or not.  RSV1 is set for compressed
      messages and unset for uncompressed messages.

Nit 3: Changed to "Thanks to" as you suggested.

---

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Tue, 23 Jun 2015 06:58:43 +0000

Hi,

Thank you for addressing my comments and clarifying the issues.

I am fine with the proposed resolutions.

Regards,

Dan

From: Takeshi Yoshino [mailto:[email-masked]]
Sent: Tuesday, June 23, 2015 9:52 AM
To: Barry Leiba
Cc: Romascanu, Dan (Dan); General Area Review Team; [email-masked]
Subject: Re: Gen-ART review of draft-ietf-hybi-permessage-compression

On Wed, Jun 17, 2015 at 12:08 AM, Barry Leiba <[email-masked]<mailto:[email-masked]>> wrote:
Hi, Dan, and thanks for the review.

-10 added hints. This is optional feature. Shouldn't impact interoperability.

-10 made it mandatory for clients to support client_no_context_takeover (at this point it was called c2s_no_context_takeover) parameter in an extension response. This shouldn't introduce any spec issue, I think.

-15 renamed s2c_ to server_ and c2s_ to client_. I think everyone has followed this change in this 1.5 year.

On the minor issues:

From 2 perspectives, I prefer not to capitalize them:
- (a) These sentences are describing what/how constraints have to be negotiated for the DEFLATE based compressed communication channel just as an example.
- (b) These sentences are not explaining requirements that implementors must follow for interoperability, but just things they have to do to make DEFLATE based compression/decompression work correctly.

Because of (b), I'd like to keep these "must keep"s uncapitalized. I'm ambivalent about (a) regarding "MUST" and "may" the following sentences

We can consider these to be kind of quotes of conformance requirements. Actually, we're defining them later in the same doc. So, I'd like to keep this MUST and change the "may" in the second sentence to "MAY" for consistency. Does this look good to you?

----

----

Nit 2: Added explanation of the significance of the values 0 and 1 as follows:

   Meaning

---

From: Takeshi Yoshino <[email-masked]>
Date: Wed, 24 Jun 2015 14:04:32 +0900

Thanks. Addressed in -23

Diff:
https://www.ietf.org/rfcdiff?url2=draft-ietf-hybi-permessage-compression-23
Summary:
https://mailarchive.ietf.org/arch/msg/hybi/7NxhaWKJaU-qSULQXQ3hnw8peM4

Takeshi

On Tue, Jun 23, 2015 at 3:58 PM, Romascanu, Dan (Dan) <[email-masked]>
wrote:

> *From:* Takeshi Yoshino [mailto:[email-masked]]
> *Sent:* Tuesday, June 23, 2015 9:52 AM
> *To:* Barry Leiba
> *Cc:* Romascanu, Dan (Dan); General Area Review Team;
> [email-masked]
> *Subject:* Re: Gen-ART review of draft-ietf-hybi-permessage-compression
>
>
>
> -10 made it mandatory for clients to support client_no_context_takeover
> (at this point it was called c2s_no_context_takeover) parameter in an
> extension response. This shouldn't introduce any spec issue, I think.
>
>
>