IETF-SSH archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: filexfer-07
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
Home |
Main Index |
Thread Index |
Old Index