Subject: Re: filexfer-07

From: Joseph Galbraith <[email-masked]>
Date: Fri, 01 Apr 2005 14:59:06 -0700

der Mouse wrote:
> I just looked over draft-ietf-secsh-filexfer-07.txt and have some
> comments.

Thanks, I appreciate the review.

> Section 3, before the beginning of 3.1, says that "Implementations MUST
> ignore excess data after an otherwise valid packet", but it is not
> clear exactly what this means: it could be taken to mean that extra
> data may appear after a valid [length;type;request-id;data] blob, or
> whether it means that extra data may appear after the data within such
> a blob.  Because of the difficulty of packetizing the octet stream in
> the former, I assume it's the latter, but I think the wording could do
> with clarification.

Thanks.  I've changed it to read:

    Implementations MUST ignore excess data at the end of an otherwise
    valid packet.  Implementation MUST respond to unrecognized packet
    types with an SSH_FX_OP_UNSUPPORTED error.  This will allow the
    protocol to be extended in a backwards compatible way as needed.

Is this better?

> In section 3.2, two is hardly "several"; I'd suggest just "This
> document defines these data types in addition...".

Done.

> In section 3.2, in the definition of "extension-pair", should it read
> s/consensus/CONSENSUS/ as is done elsewhere RFC2434 language is used?
> 
> In 3.3, RESERVED_FOR_EXTENSIONS is not a packet type value and I would
> argue it should not appear in the list of packet type values, at least
> not in a way that makes it look like a value.  Perhaps instead of
> 
> 
>    In addition, values 210 through 255 are reserved for extensions as
>    defined in Section {whatever}.

Thanks. Done.

>    Additional packet types should only be defined if the protocol
> 
> Also, the last-quoted line above suffers from the misplaced-only
> problem; it should be "...should be defined only if the..." or, in this
> particular case, perhaps even "...defined only when the...".

Actually, this conflicts with the new 'excess data' wording.
I've fixed this to clarify that servers MUST respond with
UNSUPPORTED, and that we have to rev the protocol number
if the client is to receive new packet types.  (Although
the client can receive a new packet type in response
to a new packet type request.)

> Section 4, first paragraph, ast line: "...adhere to particular versino
> of..." needs s/to par/to that par/.

Fixed.

> In 4.1, I'd rather see s/dis-contiguous/discontiguous/.

Done.

> 4.2, last sentence: s/name/&s/

Done.

> 4.3, second paragraph, last sentence: \r\n is C notation; I'd rather
> see this read something like "...uses CRLF pairs as the...".  Similar
> notation exists in the discussion of the "newline" extension.

Fixed.

> 4.3, third paragraph, reads "Servers for systems blah blah or systems
> blah blah, MUST translate...".  I'd get rid of the comma, or add
> another comma just after the parenthetical note.  But I'd prefer to
> replace the whole sentence with something like "Servers for systems
> using other conventions MUST translate to and from the canonical
> form.".  (Note I added "and from" as well.)

Done.  I used your preferred language.

> 4.4, second sentence, last paragraph: this needs a comma between
> "following extension" and "which all".

I reworded it to not be such a complex compound sentance.

> 4.4, in the description of max-read-size, has another misplaced "only":
> s/only complete/complete only/.  The second paragraph of this
> description is technically inconsistent; it says that a server MUST do
> something, and then describes cases under which it might not.  I think
> it needs a little rewording.

Done.

> 5, describing filename-translation-control, perhaps should indicate
> which value of do-translate indicates which action.  Perhaps "MUST
> enable or disable filename translation according as 'do-translate' is
> true or false"?

Done.

> 6 describes a "new compound data type is defined for encoding file
> attributes", but does not give it any name.  By exclusion and
> back-reference, I assume this is the ATTRS that appears in (eg) 7.1.1;
> I'd prefer to see the ATTRS name shown here in 6 as well.

Done.

> 6 describes the *time_nseconds fields as "if flag SUBSECOND_TIMES", but
> 6.7 makes it clear they really are "if flags {ACCESS,CREATE,MODIFY}TIME
> and SUBSECOND_TIMES".  I'd prefer this be stated clearly here, perhaps
> by indenting the "if flag" clauses:
> 
>        int64    atime                  if flag ACCESSTIME
>        uint32   atime_nseconds                 if flag SUBSECOND_TIMES

Done.

> 6 inconsistently names attributes with dashes or underscores separating
> words (eg, allocation-size, mime-type, but atime_nseconds,
> extended_count).  I'd prefer to see this uniform (I'd rather see
> dashes, but I'd rather see uniform underscores than the mixed mess
> that's there).

Done.

> 
> 6 describes a createtime field.  Based on the close match of the rest
> of the attributes, and the lack of a last-change-time field, this
> appears to be an example of the common misunderstanding of the ctime
> stat() field: it is not a create time, but a last-inode-change-time.
> I'm not sure how fixable this is, though I would like to see somewhere
> to put the ctime.

No, it is definitely createtime (that is why I didn't call it ctime!)
Windows (and other non-unix OS's) track create time as well.

I have added the following field:

    'ctime' contains the last time the file attrbutes were changed.  The
    exact meaning of this field depends on the server.

> 6.4 says that "the number of bytes tha tthe file consumes on disk" "is
> greater than or equal to [the EOF location]".  This appears to mean
> that it cannot be used on filesystems supporting holes in files, and
> must be artifically increased, or not provided, for files containing
> sufficient holes to outweigh any indirect blocks or other overhead.
> Since I assume this is supposed to be a palce to report the st_blocks
> value, I suspect this is not what's intended.

You are correct; I fixed it to explicitly note that it can be smaller
than size.

> 6.4, second paragraph, I'd prefer s/removed/& (if it was created)/,
> since it's possible that preallocation and creation may be an atomic
> operation.

Done.

> 6.4, third paragraph, last sentence: "If the operation succeeds, it
> should be the min of the 'size' before the operation and the new
> 'allocation-size'.".  (1) I'd prefer s/min/&imum/; (2) it's not clear
> what the "it" is referring back to.  I conjecture it's the 'size' of
> the file, since that's the subject of the previous sentence, but it
> appears to be referring to "the operation" from the "If" clause.

Thanks.  Fixed.

> It's not clear what to do if the same operation tries to set the size
> and the allocation-size to different values.

Hmm...

Different values is okay:

size: 400 bytes
allocation-size: 4K

Set the EOF to 400 bytes, and pre-allocate to 4K on disk.

I've added some text specifying what to do if they conflict.

I.e.,

size: 4K
allocation-size: 1K

is an error (INVALID_PARAMETER)

> 6.5, second paragraph, third line, appears to be missing an open ".

Thanks.

> 6.5, second paragraph: "...should not place any special meaning with
> the attribute value" - I'd s/with/on/.

Done.

> 6.5, last paragraph: I'd add "for a modification operation" to the end
> of the sentence.

Done. (I used 'during a modification...')

> 6.6, second paragraph: s/posix/POSIX/.

Done.

> 6.9, describing SSH_FILEXFER_ATTR_FLAGS_SYSTEM: "The file is part of
> operating system".  There is an article missing here after "of".

Fixed.

> 6.9, describing SSH_FILEXFER_ATTR_FLAGS_CASE_INSENSITIVE: another
> mispalced "only".  s/can only apply/applies only/

Done.

> 6.9, describing SSH_FILEXFER_ATTR_FLAGS_APPEND_ONLY: another misplaced
> "only".  "The file can be opened for writing only in append mode."

Hmm... someone requested that this be added for unix; I don't recall
who, and it isn't in man stat for my fedora 3 system.

I assume it means this, which is the new text:

       Opening the file without either the SSH_FXF_ACCESS_APPEND_DATA or
       the SSH_FXF_ACCESS_APPEND_DATA_ATOMIC flag (Section 7.1.1.3) MUST
       result in an SSH_FX_INVALID_PARAMETER error.

> 6.9, describing SSH_FILEXFER_ATTR_FLAGS_IMMUTABLE, first paragraph:
> missing comma after "created to this file".

Fixed.

> 6.10, section heading should be "text-hint", since that's what it's
> called above.

Thanks.

> 6.10 says that "If this flag is present during an fsetstat operation,
> the file handle is converted to a text-mode handle, as if it had been
> opened with SSH_FXF_ACCESS_TEXT_MODE"; this seems wrong if the value
> given is one of the _BINARY values, especially KNOWN_BINARY.

I've removed this text and forbidden this flag during either fsetstat
or setstat (unless someone screams _right now_!)

> 6.11, section heading should be "mime-type".

Done.

> 6.12, section heading should be "link-count".

Done.

> There is no description of the untranslated-name field.

Good catch.

I added it.

I also realized I did need the attrib-bits-valid (which I'd
put in and taken out between 06 and 07) because otherwise
the server could end up twiddling bits inadvertantly when
trying to comunicate the TRANSLATION_ERR.

> In 6.13, the edict that an implementation "SHOULD ignore" extended data
> fields it doesn't understand seems to conflict with the general dictum
> that unsupported attributes in a set operation should produce errors.
> What's the thinking behind this?

It does conflict, doesn't it?

I've changed the "supported2" extension to explicitly
declare attribute extensions versus 'SSH_FXP_EXTENDED'
extensions, and changed it to fail with SSH_FX_UNSUPPORTED

> 7.1.1.2, first paragraph, refers to section 5.7, which as far as I can
> tell does not exist.  This appears to be intended to refer to what is
> now 6.8.  Furthermore, as far as I can see, nowhere are the actual
> semantics of the various access types defined, either for open purposes
> or for ACL purposes (though in the latter case there is a reasonably
> strong implication that they should be interpreted per NFSv4).

Okay, I noted that the meaning of the flags was also in RFC3010,
fixed the reference, and emphasised that the meaning for open
was also in RFC3010.

> 7.1.1.2, second paragraph: s/it's/its/ and s/equivilants/equivalents/.
> I'd also s/can not/cannot/, but that's less important.

Done.

> 7.1.1.2, last paragraph: s/limitted/limited/ (twice).

Done.

> 7.1.1.3, describing SSH_FXF_TEXT, second paragraph: I'd remove "both
> the" and s/function/&s/.

Done.

> 7.1.1.3, describing SSH_FXF_TEXT, third paragraph: this contains two
> "correctly"s, one of which should go.

Fixed.

> 7.1.1.3, describing SSH_FXF_TEXT, describing "text-seek": does this
> really belong here?  It seems odd to describe this nested inside this
> flag bit description.

Good point... I haven't quite figured out where to put it
though.

> 7.1.1.3, describing SSH_FXF_ACCESS_READ_LOCK: s/gaurantee/guarantee/
> (twice).  Also, it's not clear whether "the exclusive reader" means
> with respect to other sftp clients or with respect to all other OS
> activity; and, an exclusive read lock is a very peculiar thing, and not
> at all what a "read lock" normally does.

All right; I renamed it to SSH_FXF_ACCESS_EXCLUSIVE_READ,
and changed it to say:

   The server MUST guarantee that no other handle has been
   opened with ACE4_READ_DATA access, and that no other
   handle will be opened with ACE4_READ_DATA access until
   the client closes the handle.  (This MUST apply both
   to other clients and to other processes on the server.)

   If there is a conflicting lock the server MUST return
   SSH_FX_LOCK_CONFLICT.  If the server cannot make the locking
   guarantee, it MUST return SSH_FX_OP_UNSUPPORTED.

   Other handles MAY be opened for ACE4_WRITE_DATA or any other
   access, as long as it does not include ACE4_READ_DATA.

Is this better?

> 7.1.1.3, describing SSH_FXF_ACCESS_WRITE_LOCK: s/gaurantee/guarantee/.
> Also, it's not clear whether "the exclusive writer" means with respect
> to other sftp clients or with respect to all other OS activity.

I used the same text as for EXCLUSIVE_READ, modified
appropriately. I also changed the name of the to EXCLUSIVE_WRITE.
> 
> 7.1.1.3, describing SSH_FXF_ACCESS_DELETE_LOCK: s/gaurantee/guarantee/.
> Also, it's not clear whether the guarantee applies to the file itself
> or to the name with which it aws opened.

I replaced the text with this:

   The server MUST guarantee that no other handle has been
   opened with ACE4_DELETE access, opened with the
   SSH_FXF_ACCESS_DELETE_ON_CLOSE flag set, and that no other
   handle will be opened with ACE4_DELETE access or with the
   SSH_FXF_ACCESS_DELETE_ON_CLOSE flag set, and that the file
   itself is not deleted in any other way until the client
   closes the handle.

Is this better?

> 7.1.1.3 says that "The 'attrs' field is ignored if an exiting file is
> opened" - this needs s/exiting/existing/.

Done.

> 7.1.1.3, "The following table is provided to assist in mapping posix
> semantics" - s/posix/POSIX/.

Done.

> 7.1.3 mentions that a close can fail, but it does not indicate what
> happens if so - interpreted under the usual assumption that a failed
> operation did not happen, it would appear that there is now a handle
> that is still open but which cannot be accessed (since the handle goes
> invalid from the client's POV upon sending the request).

True.  I believe this problem extends to close failure
at the OS level to.

I don't think there is anything the client or protocol can
do about this.

I've added the following paragraph:

    Note that the handle is invalid regardless of the SSH_FXP_STATUS
    result.  There is no way for the client to recover a handle that
    fails to close.  The client MUST release all resources associated
    with the handle regardless of the status.  The server SHOULD take
    whatever steps it can to recover from a close failure and to ensure
    that all resources associated with the handle on the server are
    correctly released.

> 7.2.1, last paragraph: I'd refer to see this worded differently.
> Perhaps "If the server returned 'max-read-size' in the "supported2"
> extension in its version packet (see section 4.4), then failure...".

I changed it to:

       If the server specified a non-zero 'max-read-size' in it's
       'supported2' (Section 4.5) extension, then failure to return
       'length' bytes indicates that EOF or an error occured.

> 7.2.2, describing "handle": s/oridinary/ordinary/

Fixed.

> 7.2.3, describing writing beyond EOF, says that it writes "zeroes".  It
> is not clear whether this is all-bits-0 bytes or the character 0 in
> some character set or what; I'd prefer to see this made clearer.

Fixed.

> 7.5 requires that SSH_FXP_FSTAT return an invalid-handle error for
> directory handles.  I think this should be fixed.

Should FSTAT work on directory handles then?

I've changed it to do so, and made it explicit that it
does.  Should FSETSTAT also work?  I've changed it to
do so.

> 7.6, last paragraph, last sentence: s/client/&s/.

Fixed.

> 7.7 says that SSH_FXP_LINK creates a symbolic link, but the description
> makes it clear that it is also for creating hardlinks as well.  Maybe
> remove "symbolic" from the description sentence?

Done.

> 7.7, last paragraph, last sentence: s/server/&s/.

Fixed.

> 7.8, describing control-byte, third or fourth paragraph:
> s/syntaticly/syntactically/.

Fixed.

> 7.8.1, RFCEDITOR note, first paragraph: s/it's/its/ - I don't know if
> these paragraphs are worth fixing; I include this in case they are.

Fixed.

> 8 describes "error message" and conflates the description of "language
> tag" into it.  I'd prefer to see "language tag" described on its own,
> parallel to the way "error message" and "error/status code" are.

Done.

> 8, describing SSH_FX_NO_CONNECTION and SSH_FX_CONNECTION_LOST: more
> misplaced "only"s.  They should be moved to after their "locally"s.

Ditched the only's; they confuse me :-)

> 8, describing SSH_FX_QUOTA_EXCEEDED: s/users/user's/

Fixed.

> 8, describing SSH_FX_UNKNOWN_PRINCIPLE: this needs to be
> SSH_FX_UNKNOWN_PRINCIPAL, with s/princple/principal/ in the
> description.  (Since the wire protocol does not use the names, such a
> change would be backward-compatible.)

Fixed.

> 8, describing SSH_FX_BYTE_RANGE_LOCK_CONFLICT and
> SSH_FX_BYTE_RANGE_LOCK_REFUSED, speaks of "byte range" locks.  As far
> as I can tell, this is the only place in the entire document that such
> a thing is mentioned.  I have no idea why two error codes were
> allocated to refer to conflicts with something that is undefined and
> outside the scope of the protocol anyway, but it is not at all clear
> what this is all about.

Some operating systems support byte range locks.  It is possible
for an SFTP operation to come into conflict with such a lock even
though the SFTP protocol does not support a byte-range lock.

If replaced the text with:

       Some operating systems support locking a range of bytes within a
       file.  On such operating systems, it is possible for a SFTP
       request to fail due to some other process owning a byte-range
       lock, even though the SFTP protocol does not support byte-range
       locks natively.

       A read or write operation failed because another process's
       byte-range lock overlaps with the request.

SSH_FX_BYTE_RANGE_LOCK_REFUSED isn't strictly necessary, but
I don't think it hurts either-- we aren't exactly constrained
by a small error space here.

I'll ditch it if people really don't want it there.

> 8, describing the end-of-file field in an SSH_FXP_DATA response:
> s/limitted/limited/.

Fixed.

> 8, describing SSH_FXP_NAME responses, says that "the remaining fields
> repeat 'count' times", but this is true of only the next two fields,
> not of the end-of-list optional field, which never repeats.

Fixed.

> 9, describing "request-specific data": another misplaced "only".  It
> needs to move to just before the "if".

Reworded to use SHOULD NOT instead, which is clearer anyhow.

> 9, describing how to allocate packet types for extensions: "in
> additional to any other data" - s/additional/addition/.	

Done.

> 9.1.2, describing "block-size": s/ever/&y/.  Also, s/entire range/&,/.

Done.

> 9.1.2, last paragraph (part of the SSH_FXP_EXTENDED_REPLY "hash"
> description): s/computer/computed/.

Fixed.

> 9.2, last paragraph ("bytes-per-allocation-unit" description):
> s/block/&s/.

Fixed.

> 9.3: I'd prefer to see it explicitly stated that the returned pathname
> is suitable for use in operations such as realpath.

Done.

> 10, third paragraph: s/channels/channel's/.

Done.

> 11, second paragraph, first sentence: swap "only" with "constrained".

Done.

> 11, seventh paragraph: while the text as it stands is not really
> _wrong_, I'd prefer to s/insure/ensure/.

Done.

> 13, isn't the canonical tardemark sentence shorter now?

Yep.  The short version is in there too :-)

Now fixed.

Thanks for the exhaustive review.

Joseph