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

From: Elwyn Davies <[email-masked]>
Date: Fri, 23 Dec 2005 11:26:16 +0000

Apologies for not replying sooner.. I was away on an extended holiday 
and missed this one in the large drfift of email that had accumulated by 
the end.

Anyway, the suggested changes sound sensible and address my concerns.  
I'll take a look at the revised draft when it appears.

Merry Christmas,
Elwyn

Alexey Melnikov wrote:

> Elwyn Davies wrote:
>
>>  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
>
>
> 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 
>
> and needs a capability identifier.
>
>>
>>  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.
>
>>  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/
>
>
> All changed.
>
>>  s2.6.2: s/is immediately followed by/may contain/
>
>
> I've changed that to "starts with".
>
>>  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)
>
>
> All changed.
>
>>  [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.
>
>
> Already updated in my copy.
>
> Thanks for the comments!
> Alexey
>

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