Copy-edit comments on filexfer-08

[der Mouse <mouse%Rodents.Montreal.QC.CA@localhost>]

> 	Filename	: draft-ietf-secsh-filexfer-08.txt

I finally got around to doing a copy-edit read over this.

As compared to filexfer-07:

-   channel in [I-D.ietf-secsh-architecture].  and that the server has
+   channel in [I-D.ietf-secsh-architecture]. and that the server has

This change is good, but s/\. /, / surely?

At the end of 3.3, I see new text

   Values 210-255 are reserved for use in conjunction with these
   extensions.  The SSH_FXP_EXTENDED packet can be used to negotiate the
   meaning of these reserved types.  It is suggested that the actual
   value to be used also be negotiated, since this will prevent
   collision among multiple uncoordinated extensions.

   The server MUST respond with SSH_FXP_STATUS(SSH_FX_OP_UNSUPPORTED) if
   it receives a packet it does not recognize.  The protocol version
   (Section 4) MUST be incremented if the server is to send new packets
   to the client (because the client has no way to respond indicating
   that the packet isn't recognized.)

Except for the first sentence in each of these paragraphs, they appear
to me to be in conflict: the first is saying "here's how to negotiate
new packet types in this version" and the second is saying "the
protocol version must be changed to send new packet types".

At the beginning of 4.4, I see

   It is often necessary to detect the version of a the server so that

"a the server"?  Surely that needs fixing.

Just after the page break there, there's a missing ):

   so.  (It may also be sent by the client using an EXTENDED request.

In 4.5, supported-open-block-masks and supported-block-masks are
uint64s.  Is there any reason they aren't simply 16-bit fields
(probably uint32 on the wire), with each bit indicating whether a
particular combination is supported?  (There are four BLOCK bits, thus,
16 possible combinations.)  Also, the current spec does not provide any
indication of how many of the 16 four-bit fields are valid in the
uint64; while the sender clearly must pad with multiple copies of some
combination (as in the example given, which pads with multiple copies
of 0b0000), I'd prefer to see this mentioned explicitly too -
especially in the example; the current wording implies that only the
low two nibbles are used in that example.  (Since at least one
combination must be supported for sftp to be useful at all, it's true
that there's no problem finding a combination to pad with.)  It also
occurs to me that you might want to require that 0b0000 be accepted.

There's a blank line missing between supported-block-masks's
description and attrib-extension-count's.

In 4.6, the example given for comma-separated-versions includes a
version not in the list of permitted version numbers.  I'm not sure
whether I think this needs to change or not, but it looks at least a
little odd.  Also, shouldn't "higher than version '3'" at the beginning
of the next paragraph be "at least as high as version '3'" or "no lower
than '3'" or some such?

In the version-select request, the version-from-list is a uint32.
Since version numbers may be other than simple integers (the
comma-separated-versions is defined to include the DNS extensibility
mechanism), shouldn't this be a string?  Otherwise, how is the client
supposed to indicate whether that (say) 8 it's sending is
8%rodents.montreal.qc.ca@localhost or 8%openssh.org@localhost or 8%new-ssh.example.net@localhost -
and for that matter what should the client do if the server's list
includes totally non-numeric values such as "fnord%example.com@localhost"?

Also, there is wording such as "higher than" that implies there is a
total order on version "number"s.  It isn't obvious to me that this is
true in the presence of the DNS extensibility mechanism.

In section 5,

   Servers SHOULD interpret a path name component ".."  (Section 11) as

an extra space seems to have got inserted between " and (.  I
conjecture some automated tool mistook ." for an end-of-quoted-sentence
and inserted end-of-sentence space in consequence.

In 6.9,

   comminicute only the bits it knows about without inadvertantly

s/comminicute/communicate/

There's a misplaced "only" that I apparently missed before:

   mechanism is recommended for most purposes; additional flags bits
   should only be defined by an IETF standards action that also
   increments the protocol version number.  The use of such new fields

s/only be defined/be defined only/

Various places in the document speak of SSH_FXF_TEXT.  This appears to
be undefined as to numerical value; the placement of its description in
7.1.1.3, and the lack of a description of the SSH_FXF_ACCESS_TEXT_MODE
flag which appears in the list early in 7.1.1.3, leads me to suspect
that the one is a mistake for the other.  In any case, something needs
to be fixed.

The description of SSH_FXF_TEXT (see the previous paragraph) says that

      When a file is opened with the FXF_TEXT flag, the offset field in
      the read and write functions is ignored.

but it also says

      Clients MUST be prepared to handle SSH_FX_OP_UNSUPPORTED returned
      for read or write operations that are not sequential.

Which is it?  Is the offset ignored, or is the server permitted to
check it and return OP_UNSUPPORTED errors for non-sequential I/O?

SSH_FXF_ACCESS_BLOCK_READ's description begins

      The server MUST guarantee that no other open has taken place which
      blocked handle has been opened with ACE4_READ_DATA access, and

This looks like fragments of two sentences pasted together ("no other
open has taken place which blocked ...??... handle has been opened
with").

SSH_FXF_ACCESS_BLOCK_WRITE's description begins

      The server MUST guarantee that no other lock has been opened with
      ACE4_WRITE_DATA or ACE4_APPEND_DATA access, and that no other

"no other lock has been opened"?  s/lock/handle/, or something else?

SSH_FXF_ACCESS_DELETE_ON_CLOSE appears to be intended for operations
that, in the traditional Unix model, unlink an open file.  But that
operation, while it does not delete the file, *does* delete the *name*
immediately.  It is not clear whether DELETE_ON_CLOSE is permitted to
destroy the name immediately even if it does not destroy the file,
since as far as sftp clients who do not have a file open are concerned,
the file's existence is indistinguishable from its name's existence.

7.2.1, describing the offset argument; 7.2.3's "offset" is similar:

      'offset' is the offset (in bytes) relative to the beginning of the
      file that the read MUST start at. 'offset' is ignored if
      SSH_FXF_TEXT was specified during the open.

The end-of-sentence space after "at." has shrunk.

7.2.1 again, describing "length":

      If the server specified a non-zero 'max-read-size' in it's

s/it's/its/

7.7, third paragraph, typo fix:

   The SSH_FXP_LINK request creates a link (either hare or symbolic) on
   the server.

s/hare/hard/

In 7.7, after describing SSH_FXP_LINK, the text charges right into a
description of SSH_FXP_LOCK, or SSH_FXP_BLOCK - the running text says
_LOCK but the request description says _BLOCK.  There also is no
heading break; I'd expect to see 7.8 begin here.  Also, the first
sentence seems to either have a spurious article or lack a noun:

   The SSH_FXP_LOCK creates a ...

Either s/The // or s/LOCK/& request/, it seems to me.

I also note that LOCK/BLOCK is specified as not working on directories;
I see no particular reason for the protocol to forbid this (though of
course some servers won't support it).  (Of course, it's not that
useful for non-advisory locks, but now that the protocol has advisory
locks, it does have its uses.)

The description is also missing two blank lines between offset and
length, and length and uLockMask.  Also, the studlyCaps style of
uLockMask is rather at odds with the style of most other packet field
names, which would argue for u-lock-mask instead.

SSH_FXP_UNLOCK is similar: it is confused between UNLOCK and UNBLOCK,
it has either a spurious article or a missing noun, it has no heading,
it is specced to fail on directories, and it's missing a blank line
between offset and length.

In 7.8,

   The server MUST take the 'original-path' and apply the 'compose-path'
[page-break]
   as a modification to it. 'compose-path' MAY be relative to 'original-
   path' or may be an absolute path, in which case 'original-path' will
   be discarded.  The 'compose-path' may be zero length.

s/. '/.  '/ in the second line.

I also think that if you're going to use "MAY" there, you should use it
for the other one, the one that's now spelled "may", too.

In 7.8.1, in view of the RFCEDITOR REMOVE BEFORE PUBLISHING paragraphs,
I note that the current REALPATH mechanism does have a downside which
is not described: when composing more than two pieces, multiple server
roundtrips are required.  How much of a pain would it be to make
REALPATH take a more or less arbitrary number of compose-paths?

8.1 seems to be missing a blank line between "language tag" and
"<error-specific data>".  In passing, why does the description have <>
around "error-specific data" when the packet description doesn't?

8.1, describing SSH_FX_OP_UNSUPPORTED, has a stray ) at the end of the
description.

8.1, describing SSH_FX_UNKNOWN_PRINCIPAL, missed changing one
"principle" to "principal" (the fourth-last word in the description).

8.1 says, in SSH_FX_BYTE_RANGE_LOCK_CONFLICT, that "the SFTP protocol
does not support byte-range locks natively" even though
SSH_FXP_{,B}LOCK (see above for the ambiguity) does indeed support
byte-range locks.

9.1.2, describing hash-algorithm-list:

      A comma separated list of hash alogirthms the client is willing to

s/alogirthms/algorithms/

Also in 9.1.2's hash-algorithm-list description, an end-of-sentence
space has shrunk:

      and SHA-512 are decribed in [FIPS-180-2]. crc32 is described in

In 9.3, the absolute-pathname is said to be "suitable for use in
operations such as REALPATH or OPEN" - surely s/OPEN/&DIR/?

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       mouse%rodents.montreal.qc.ca@localhost
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B