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
