filexfer-07

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

I just looked over draft-ietf-secsh-filexfer-07.txt and have some
comments.

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.

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

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

       SSH_FXP_EXTENDED          200
       SSH_FXP_EXTENDED_REPLY    201

Galbraith, et al.      Expires September 25, 2005               [Page 7]
^L
Internet-Draft         SSH File Transfer Protocol             March 2005

       RESERVED_FOR_EXTENSIONS           210-255

   Additional packet types should only be defined if the protocol

have it read

       SSH_FXP_EXTENDED          200
       SSH_FXP_EXTENDED_REPLY    201

Galbraith, et al.      Expires September 25, 2005               [Page 7]
^L
Internet-Draft         SSH File Transfer Protocol             March 2005

   In addition, values 210 through 255 are reserved for extensions as
   defined in Section {whatever}.

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

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

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

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

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.

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

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

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.

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

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.

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

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

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.

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.

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.

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.

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

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

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

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

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

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

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

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

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

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

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.

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

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

There is no description of the untranslated-name field.

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?

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

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.

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

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

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

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.

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.

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.

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.

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

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

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

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

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

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.

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

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

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?

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

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

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.

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.

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

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

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

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.

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

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.

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

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

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

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

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

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

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

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

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

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

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