Subject: [Gen-art] Gen art review: draft-melnikov-imap-ext-abnf-05.txt

From: Elwyn Davies <[email-masked]>
Date: Tue, 01 Nov 2005 11:57:57 +0000

Background for those on the CC list, who may be unaware of GenART:
GenART is the Area Review Team for the General Area of the IETF.  We
advise the General Area Director (i.e. the IETF/IESG chair) by providing
more in depth reviews than he could do himself of documents that come up
for final decision in IESG telechat.  I was selected as the GenART
member to review this document.  Below is my review, which was written
specifically with an eye to the GenART process, but since I believe that
it will be useful to have these comments more widely distributed, others
outside the GenART group are being copied.

Document: draft-melnikov-imap-ext-abnf-05.txt
Intended Status: Proposed Standard (Individual submission via AD)
Shepherding AD: Allison Mankin
Review Trigger: IETF Last Call ending 8 November 2005

Summary:
A slightly odd 'standard' this one.  I spent a few minutes wondering why this 
wasn't defining any capability names and had a 'null' IANA considerations before 
twigging that the majority (originally, back at version -00, all) of this 
document was a set of patterns (maybe a 'meta-standard') telling extension 
authors that if they needed to add options to certain commands 'do it this 
(common) way'.  I think this could be spelt out more clearly in the 
introduction, but I don't have any objection in principle to the purpose of the 
document.  It seems to be a useful set of 'class definitions' (in object 
oriented terms) that can be instantiated by real extensions and will allow for 
commonality in the parser.

However, there is (IMO) an impostor lurking in section 2.7.  The extension of 
the MULTIAPPEND capability to allow data with embedded nuls is not a class 
definition but a real extension.  I *think* this needs a capability identifier 
to be registered with IANA.  In any case it is an update of RFC3502 which is not 
mentioned at present.  I feel that this extension sits awkwardly with the rest 
of the document and it might be better hived off into a separate draft.

In addition, there are a number of minor points and editorial nits that need 
fixing up.

I haven't checked the validity of the ABNF in s3.

Review:
The abstract and introduction  (to section 2.1) should explain more clearly that 
the majority of the document provides a number of standard patterns for adding 
options and extensions to standard IMAP commands that are currently and SHOULD 
(in the future) be used by any new extensions that needs this sort of 
capability, but, except for s2.7, does not define any actual extensions. 
Section 2.7 appears to define a real extension and needs (IMO) a capability name.

Issues:
Abstract:  As it stands the document updates RFC3502 (MULTIAPPEND) by allowing 
for nuls in data (and rewriting the ABNF).  This needs to be mentioned.

Abstract: As far as I can understand, the *addition* of the <literal8> 
production does not update RFC2088 but merely draws on the syntax used to define 
a similar format.  I think RFC2088 can be removed from the list of updated RFCs.

Abstract: The abstract says that RFC2342 (NAMESPACE) is updated by this draft. 
If my (manual) parsing of the ABNF is correct, the ABNF is equivalent with just 
one extra 'convenience' production added in to make the display clearer.  Does 
this *really* constitute an update?  The *usage* of the namespace-response in 
the alternative mailbox-data rules doesn't seem to update RFC2342, but rather 
(probably) require that the server had this capability to be able to support 
this option.  Maybe this needs some extra explanation.

s2: This list of updated RFCs (not including RFC2088) should be reiterated and 
given as references.

s2.1: Needs some pointers to the pieces of RFC3501 that are updated (as in s2.2 
etc).

s2.6: Should there be a section describing the namespace-response as a new 
alternative response (as with the ESEARCH response)? The introduction of RFC2342 
stuff is not otherwise talked about except in the ABNF.

s2.6.1: According to the ABNF comments, the response to SEARCH should be exactly 
one instance of either a SEARCH response or an ESEARCH response... I think the 
'Responses' line in s2.6.1 should be saying this.  The 'footnote' attached to 
the SEARCH response is not very clear .. it should probably say explicitly that 
the ESEARCH response is a possibility and that there should be exactly one 
response (i.e., SEARCH and ESEARCH responses are mutually exclusive).

s2.7: This section should have a ref to [MULTIAPPEND] (RFC3502).  It should 
probably also remind implementers that it interacts with RFC2359 (and now 
2359bis).  As stated above I believe that this is defining an actual capability 
and needs a capability identifier.

s4: The extra security considerations in RFC2342 also apply.

s5: There may be IANA considerations if my view on s2.7 is correct.

Editorial nits:
===============
global: s/don't/does not/
global: s/can't/cannot/

s2.1, para 1: s/behaviour/behaviours/
s2.1, bullets: s/effects/affects/ (2 places)
s2.1, bullet 2: s/For/for/

s2.6.1: s/define any result option/define any result options/

s2.6.2: s/is immediately followed by/may contain/
s2.6.2: s/missing than/missing then/
s2.6.2: s/if it is present than/whereas if it is present/
s2.6.2: s/is referring/refers/ (2 places)

[Paras 3 and 4 of s2.6.2 should then read:
    The ESEARCH response may contain an optional search
    correlator. If it is missing then the response was not caused by a
    particular IMAP command, whereas if it is present it contains the tag
    of the command that caused the response to be returned.

    The search correlator is followed by an optional UID indicator. If
    this indicator is present, all data in the ESEARCH response refers to UIDs,
    otherwise all returned data refers to message numbers.
]

s6.1: [ABNF] - The new version has been published as RFC4234.

_______________________________________________
Gen-art mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/gen-art

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 28 Nov 2005 12:14:06 +0000

>  Review Trigger: IETF Last Call ending 8 November 2005
 
Hi Elwyn,
Sorry for the late response, it took me longer to digest your comments.
 
>  Summary:
>  A slightly odd 'standard' this one.  I spent a few minutes wondering 
why this wasn't defining any capability names and had a 'null' IANA 
considerations before twigging that the majority (originally, back at 
version -00, all) of this document was a set of patterns (maybe a 
'meta-standard') telling extension authors that if they needed to add 
options to certain commands 'do it this (common) way'.  I think this 
could be spelt out more clearly in the introduction,
 
I've added section 1.1 that talks about this in more details.
 
>  but I don't have any objection in principle to the purpose of the 
document.  It seems to be a useful set of 'class definitions' (in object 
oriented terms) that can be instantiated by real extensions and will 
allow for commonality in the parser.
 
Exactly!
 
>  However, there is (IMO) an impostor lurking in section 2.7.  The 
extension of the MULTIAPPEND capability to allow data with embedded nuls 
is not a class definition but a real extension.  I *think* this needs a 
capability identifier to be registered with IANA.
 
The extension to APPEND command to allow binary data is already defined 
in RFC 3516. However after your comment I've realized that the reference 
to RFC 3516 is missing.
The first sentence of section 2.7 now reads:
 
 IMAP BINARY extension [BINARY] extends the APPEND command to allow the 
client to append data containing NULs by using the <literal8> syntax.
 
Is this sufficient to address your concern?
 
>  In any case it is an update of RFC3502 which is not mentioned at present.
 
Added the reference, thanks.
 
>  I feel that this extension sits awkwardly with the rest of the 
document and it might be better hived off into a separate draft.
>
>  In addition, there are a number of minor points and editorial nits 
that need fixing up.
>  I haven't checked the validity of the ABNF in s3.
 
I've verified it using ABNF parsers.
 
>  Review:
>  The abstract and introduction  (to section 2.1) should explain more 
clearly that the majority of the document provides a number of standard 
patterns for adding options and extensions to standard IMAP commands 
that are currently and SHOULD (in the future) be used by any new 
extensions that needs this sort of capability,
 
I've added 2 paragraphs to the abstract:
 
This document also suggest a set of standard patterns for adding options 
and extensions to several existing IMAP commands defined in RFC 3501. 
The patterns provide for compatibility between existing and future 
extensions.
 
This document updates ABNF in RFC 3501, RFC 2088, RFC 2342, RFC 3502 and 
RFC 3516. It also includes part of the errata to RFC 3501. This document 
doesn't specify any semantic changes to the listed RFCs.
 
>  but, except for s2.7, does not define any actual extensions. Section 
2.7 appears to define a real extension and needs (IMO) a capability name.
 
See above.
 
>  Issues:
>  Abstract:  As it stands the document updates RFC3502 (MULTIAPPEND) by 
allowing for nuls in data (and rewriting the ABNF).  This needs to be 
mentioned.
 
Done.
 
>  Abstract: As far as I can understand, the *addition* of the <literal8> 
production does not update RFC2088 but merely draws on the syntax used 
to define a similar format.  I think RFC2088 can be removed from the 
list of updated RFCs.
 
When RFC 2088 was written there was no extension to allow for 8 bit 
data. Such extension was defined later on in RFC 3516.
However RFC 3516 doesn't update syntax of RFC 2088. This draft tries to 
address the issue by combining the two syntaxes together in a single ABNF.
 
>  Abstract: The abstract says that RFC2342 (NAMESPACE) is updated by 
this draft. If my (manual) parsing of the ABNF is correct, the ABNF is 
equivalent with just one extra 'convenience' production added in to make 
the display clearer.
 
That is correct. I've added new paragraph about that (see below my 
comments about section 1).
 
>  Does this *really* constitute an update?
 
I don't know :-). The document says "update to ABNF".
 
>   The *usage* of the namespace-response in the alternative mailbox-data 
rules doesn't seem to update RFC2342, but rather (probably) require that 
the server had this capability to be able to support this option.  Maybe 
this needs some extra explanation.
 
I've added comment to the namespace-response rule saying that only 
servers that support RFC 2342 should send it.
 
>  s2: This list of updated RFCs (not including RFC2088) should be 
reiterated and given as references.
 
I've added subsection 1.1 to describe in more details purpose of the 
document. The new section reads:
 
This document serves several purposes:
 
1.  rationalize and generalize ABNF for some existing IMAP extensions;
 
2.  collect the ABNF in one place in order to minimize cross references 
between documents;
 
3.  define building blocks for future extensions so that they can be 
used together in a compatible way.
It is expected that a future revision of this document gets incorporated 
into a revision of RFC 3501.
 
This document updates ABNF in RFC 3501, RFC 2088, RFC 3502 and RFC 3516. 
It also includes part of the errata to RFC 3501. This document doesn't 
specify any semantic changes to the listed RFCs.
 
The ABNF in section 6 of RFC 2342 got rewritten to conform to the ABNF 
syntax as defined in RFC 4234 and to reference new non-terminals from 
RFC 3501. It was also restructured to allow for better readability. 
There were no changes "on the wire".
 
Section 2 extends ABNF for SELECT, EXAMINE, CREATE, RENAME, FETCH /UID 
FETCH, STORE/UID STORE, SEARCH and APPEND commands in a consistent 
manner. Extensions to all the commands but APPEND have the same 
structure. Extensibility for the APPEND command was done slightly 
differently in order to preserve backward compatibility with existing 
extensions.
Section 2 also defines a new ESEARCH response which purpose is to define 
a better version of the SEARCH response defined in RFC 3501.
 
Section 3 defines the collected ABNF that replaces pieces of ABNF in the 
aforementioned RFCs. The collected ABNF got generalized to allow for 
easier future extensibility.
 
>  s2.1: Needs some pointers to the pieces of RFC3501 that are updated 
(as in s2.2 etc).
 
I've added references to the relevant sections of RFC 3501. Thanks!
 
>  s2.6: Should there be a section describing the namespace-response as a 
new alternative response (as with the ESEARCH response)?
 
I don't think so, because the draft doesn't change anything in RFC 2342, 
it is only rewriting NAMESPACE command/response ABNF to make it more 
extensible.
 
>  The introduction of RFC2342 stuff is not otherwise talked about except 
in the ABNF.
>
>  s2.6.1: According to the ABNF comments, the response to SEARCH should 
be exactly one instance of either a SEARCH response or an ESEARCH 
response... I think the 'Responses' line in s2.6.1 should be saying 
this.  The 'footnote' attached to the SEARCH response is not very clear 
.. it should probably say explicitly that the ESEARCH response is a 
possibility and that there should be exactly one response (i.e., SEARCH 
and ESEARCH responses are mutually exclusive).
 
I've added to the footnote in section 2.6.1:
 
Section 2.6.2 defines a new ESEARCH untagged response, that replaces the 
SEARCH untagged response. Note that for a given extended SEARCH command 
the SEARCH and ESEARCH responses SHOULD be mutually exclusive, i.e. only 
one of them should be returned.
 
>  s2.7: This section should have a ref to [MULTIAPPEND] (RFC3502).
 
I've added the following text:
 
In addition, the non-terminal "literal8" defined in [BINARY] got 
extended to allow for non-syncronizing literals if both [BINARY] and 
[LITERAL+] extensions are supported by the server.
 
The IMAP MULTIAPPEND extension [MULTIAPPEND] extends the APPEND command 
to allow a client to append multiple messages atomically. This document 
defines a common syntax for the APPEND command that takes into 
considerations syntactic extensions defined by both [BINARY] and 
[MULTIAPPEND] extensions.
 
>  It should probably also remind implementers that it interacts with 
RFC2359 (and now 2359bis).
 
I think RFC 2359 is out of scope for the document, as the document 
doesn't try to incorporate ABNF for all existing IMAP extensions.
 
>   As stated above I believe that this is defining an actual capability 
>  s4: The extra security considerations in RFC2342 also apply.
 
I've added the following sentence to section 4:
 This document updates ABNF in RFC 3501, RFC 2342, RFC 2088, RFC 3502 
and RFC 3516. The updated documents must be consulted for security 
considerations for the extensions that they define.

>  s2.6.1: s/define any result option/define any result options/
 
All changed.
 
>  s2.6.2: s/is immediately followed by/may contain/
 
I've changed that to "starts with".
 
>  s2.6.2: s/is referring/refers/ (2 places)
 
All changed.
 
>  s6.1: [ABNF] - The new version has been published as RFC4234.
 
Already updated in my copy.

Thanks for the comments!
Alexey