Re: Copy-edit comments on filexfer-08

[Joseph Galbraith <galb-list%vandyke.com@localhost>]

Thanks to der Mouse for the reviews.  I've gone through
and fixed these.  I'm going to cycle the draft again
in a couple of days, so let me know if anything else
needs changing.

(I believe this round introduced one incompatible change.
If you are shipping or getting ready to ship a v6 implementation,
let me know... I think we are getting close to being done...)

der Mouse wrote:
> 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?

Fixed.
> 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".

Not quite.  Notice that the restriction is applied to new
packets being sent to the client, not the other way around.

The client _can_ send new packets.  I have, however, revised
the section to hopefully be more clear:

   The server MUST respond with SSH_FXP_STATUS(SSH_FX_OP_UNSUPPORTED) if
   it receives a packet it does not recognize.

   The use of additional packet types in the non-extension range MUST be
   introduced through IETF consensus.  New packet types to be sent from
   the client to the server MAY be introduced without changing the
   protocol version (Section 4).  Because the client has no way to
   respond to unrecognized packet types, new packet types to be sent
   from the server to the client the client MUST not used unless the
   protocol version is changed or the client has negotiated to received
   them.  This negotiation MAY be explicit, through the use of
   extensions, or MAY be implicit, by the client itself using a packet
   type not defined above.

> 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.

Fixed.

> 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?

Well, I set out to do it that way originally, but it was easier
to specify this way.  Can you come up with some language that
specs it reasonably clearly with adding a whole bunch of new
identifiers?

I also suspect that it will be easier to use as a series of
bitmasks.

> (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.

I origanally had wording specifying that it should be padded out
with 0b0000, but it got lost somewhere.

I'm still not entirely happy with this whole thing... but nothing
better is coming to mind at the moment.

>  (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.

Right; I've added that 0b0000 MUST be accepted, and that no other
mask other than 0b0000 may occur more than once.

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

Thanks, fixed.

> In 4.6, the example given for comma-separated-versions includes a
> version not in the list of permitted version numbers.

Fixed.

  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?

Fixed.

> 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?

Yikes!  Thanks, indeed.  It is a string (and the uint32 version
is left over from a prior proposal.)

> 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.

Pedantically, what you say is true; however, there is order (at
least in time and features set) to the versions specified so
far.

I've changed it to read:

   Typically, the client SHOULD NOT down-grade the protocol version
   using this extension; however, it is not forbidden to do so.  One
   reason a client might do so is to work around a buggy implementation.

Is that better?

> 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.

Argh... I can't seem to convince it to stop.  I don't
care enough about this to do manual edits before
publishing... if the RFC editor cares, I'll do a manual
edit before we go RFC :-)

> In 6.9,
>
>    comminicute only the bits it knows about without inadvertantly
>
> s/comminicute/communicate/

Thanks.

> 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/

Thanks.

> 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?

Thanks... I took out the second one-- the offset is ignored.

> 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?

Thanks, fixed both of them.

> 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.

Hmmm... in this case, I think I'm inclined to make it undefined
whether the filename is removed immediately or on close.  (In
windows, I believe neither the name nor the file is removed until
close.)

Does anyone think this is a problem?

> 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.

Thanks.  Fixed to be a little less awkward to (hopefully.)

> 7.2.1 again, describing "length":
>
>       If the server specified a non-zero 'max-read-size' in it's
>
> s/it's/its/

Thanks.

> 7.7, third paragraph, typo fix:
>
>    The SSH_FXP_LINK request creates a link (either hare or symbolic) on
>    the server.
>
> s/hare/hard/

Thanks.

> 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.

Fixed.

> 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.)

Fixed.

> 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.

Fixed.

> 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.

Fixed.

> 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.

Argh... xml2rfc is doing this... I'm going to ignore it for now...
maybe eventually, it will get smarter?

> 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.

Done.
> 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?

Done.

I switched the order of the control byte and compose path and
let compose path have multiple elements.  Read until EOP.

> 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?

Fixed; and removed <>.

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

Thanks.

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

Thanks.

> 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.

It's gone now.

> 9.1.2, describing hash-algorithm-list:
>
>       A comma separated list of hash alogirthms the client is willing to
>
> s/alogirthms/algorithms/

Fixed.

> 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

xml2rfc has a grudge.  I reordered the sentance to make it
do the right thing (TM)

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

Fixed.

Thanks again for you reviews.

Joseph