This is a combined Gen-ART and Transport Area review, hence
the introductory "boilerplate" for both reviews follows:
I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).
I've reviewed this document as part of the transport area
directorate's ongoing effort to review key IETF documents. These
comments were written primarily for the transport area directors, but
are copied to the document's authors for their information and to
allow them to address any issues raised. The authors should consider
this review together with any other last-call comments they receive.
Please always CC tsv-dir(_at_)ietf(_dot_)org if you reply to or forward this
review.
Please resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-nfsv4-minorversion1-26.txt
Reviewer: David L. Black
Review Date: 27 November 2008
IETF LC End Date: 27 November 2008
Summary:
This draft is basically ready for publication, but has nits that
should be fixed before publication.
Comments:
In general, this is a well written draft and reasonably readable
despite its length. The draft has had extensive review in the WG
and there are multiple known interoperable implementations of
much of its contents. As a result, this review focuses on clear
exposition of requirements.
I found one technical issue that must be addressed:
Section 5.10 on character case for Unicode references a very
old RFC (RFC 1345) and provides a case determination algorithm
(substring match on "CAPITAL" or "SMALL" in a Unicode character
name) that is wrong and potentially dangerously so. This
section needs to be rewritten to reference a Unicode document
that defines the notion of case for a particular version of
Unicode. The good news is that everything needed to get this right
is in Section 14's profiles of "stringprep" (Unicode 3.2 is
referenced from Section 14 via RFC 3454), so a small rewrite of
Section 5.10 to remove the case determination algorithm and
direct the reader to Section 14 for all internationalization
details will probably fix this. I would remove the reference
to RFC 1345 as part of this.
Minor Technical Items:
The draft is a bit cavalier about use of lower case "must" and
"should". I found a number of these that arguably should be upper
case, but nothing that struck me as crucial to change.
Section 2.9.1 - at the end of the first sentence in the last
paragraph, please add ", as a consequence UDP by itself MUST NOT
be used as an NFSv4.1 transport". This is clearly a consequence
of the stated requirements, but one that is worth calling out
explicitly.
Section 2.10.1 - I was looking for at least a "MUST support" and
possibly a "MUST use" requirement for sessions in this section
and didn't see either requirement stated. At least "MUST support"
needs to be added here.
Section 2.10.8.1 - "The connection cannot be hijacked by an
attacker who connects to the client port prior to the intended
server as is possible with NFSv4.0." I think that sentence is too
strong, and that what was meant is that NFSv4.1 provides security
measures (e.g., authentication) that enable this form of
hijacking to be prevented.
Section 2.10.9, 3rd paragraph - "the input text as represented
by the XDR encoded enumeration of type ssv_subkey4." That's
a bit too terse. Changing "enumeration" --> "enumeration value
for that subkey" would be clearer.
p.80 after item 4. - "If there is a reconfiguration event which
results in the same network being assigned to servers where the
eir_server_scope value is different," Should that be "same
network address" rather than "same network" ?
p.85 - The description of "length4" is "Describes LOCK lengths."
The length4 type is used for a lot more than locks, so the
description needs to be generalized.
p.91, Section 3.3.15 - Change "will be defined" to "are defined"
in the last sentence.
p.103, Section 5.4 - Explain what the implementation implications
are of the attribute class (per server vs. per FS vs. per FS object).
Section 5.8.* and elsewhere - consider including the data type of
the attribute in the name of the section that defines the attribute
or in the text of that section.
p 114, Section 5.8.2.30 - The last paragraph presumably refers to
selection of the quota set that provides the contents of the
quota_used attribute. That should be stated explicitly.
p.117, middle of page - "The "dns_domain" portion of the owner
string is meant to be a DNS domain name. For example,
user(_at_)ietf(_dot_)org(_dot_)" Change "ietf.org" to "example.org". See
ID-Checklist item 3.6 and RFC 2606.
p.118, end of Section 5.9 - Add a "MUST NOT" requirement
forbidding use of the "nobody" string to represent an
actual owner principal.
p.135, ACE4_SYNCHRONIZE - Explain what "synchronized reads and
writes" are synchronized with/to.
p.141, Section 6.3.1.1 - Add a bullet item indicating that retention
may also block access otherwise allowed by ACLs with a pointer
to Section 5.13.
p.196, near top of page - "The implementor is cautioned in this
approach." This needs be rewritten as a requirement that clients
SHOULD NOT or MUST NOT do what has been described, because doing
so may cause data corruption.
Editorial Nits:
p.14 - The definition of "Stable Storage" doesn't actually
define the term. Rephrase start of definition to "Storage
from which data stored by an NFSv4.1 server can be recovered
without data loss after ..." plus change "and hardware
failures" to "and/or hardware failures".
p.15 - Section 1.6, second sentence, change "distinguish
those added" to "distinguish those features added".
p.15 - Section 1.6.1, this should be in present tense, so
change first two instances of "will be used" to "is used"
and the third one to "are used".
Section 2.7 - the minor versioning rules contain multiple
instances of "may not". These should all be changed to
"must not".
p.241 - " The consequences of having no facilities available
to reclaim locks on the sew server will depend on the type of
environment." sew --> new .
I found a number of phrasing nits that I will leave to the
RFC Editor to deal with.
idnits 2.10.02 flagged a lot of things that don't need attention,
but found a few that do:
- "RFC 2119" is missing from reference [1].
- The boilerplate should be updated if the draft is revised.
- Reference [19] could use some more elaboration. Would a URL be
appropriate?
- There's now a -10 version of the pnfs block layout draft.
Thanks,
--David
----------------------------------------------------
David L. Black, Distinguished Engineer
EMC Corporation, 176 South St., Hopkinton, MA 01748
+1 (508) 293-7953 FAX: +1 (508) 293-7786
black_david(_at_)emc(_dot_)com Mobile: +1 (978) 394-7754
----------------------------------------------------
_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf