Subject: Some issues in the spec

From: "Noveck, Dave" <[email-masked]>
Date: Sat, 4 May 2002 09:01:07 -0700

I started looking at the spec to find out the proper 
error code for something and wound up finding a bunch 
of stuff that I think needs to be fixed somehow in 
the spec.

Issue 1) What error is correct if, for example, you 
try do do a write on a file and there is a conflicting 
lock?  Section 8.1.4 talks about returning NFS4ERR_LOCKED 
in that case, but the context of the paragraph is talking
about stateid of all 0's, so it is not clear if this
holds for all cases.  (I think it should).  The error
list for READ and WRITE has NFS4ERR_LOCKED but also
NFS4ERR_DENIED and it is unclear in what cases that
latter error would be returned.  DENIED is defined as
involving being unable to get a lock which READ and WRITE
don't really do.  You could argue that they get an "implicit"
lock which would justifiy DENIED but then what is LOCKED
for?  Seems like there is room for DENIED or LOCKED
but not both.  (just noticed that SETATTR has DENIED but
not LOCKED)

Proposal 1) Get rid of DENIED under READ and WRITE and
revise 8.1.4 to make clear that LOCKED applies to all
cases of a conflicting lock with READ and WRITE.  Get
rid of DENIED under SETATTR amd add LOCKED.

or the reverse if we decide that DENIED is better.

Issue 2) The last sentence of the second paragraph of
section 8.1.4 doesn't seem to make sense.  The previous
sentence says that writes with all-1 stateid's do not
bypass record locking checks.  So what about share
locking checks?  Are they bypassed or not?  It just
says they're handled by the open operation which doesn't
seem to be all that relevant given that we are talking
about a special stateid for which there presumably was
no open.  If you did an open, you'd have a real stateid
and the all-1's stateid would have no purpose (at least
for write since it doesn't override locking checks for
write).

Proposal 2) Scratch "record" in the first and second sentences
of this paragraph and get rid of the third sentence entirely.

Issue 3) SETATTR has a stateid to deal with the fact that
changes in size are modifying data just a write does, but 
this isn't explained very clearly

Proposal 3) Replace the second paragraph under DESCRIPTION for
SETATTR by the following:

The stateid argument for SETATTR is used to provide file
locking context that is necessary for SETATTR requests that
set the size attribute.  Since setting the size attribute 
modifies the file's data, it has the same locking requirements
as a corresponding WRITE.  Any SETATTR that sets the size
attribute is incompatible with a share lock that specifies
DENY_WRITE.  The area between the old end-of-file and the new
end-of-file is considered to be modified just as would have 
been the case had the area in question been specified in
WRITE, for the purpose of checking conflicts with record locks.
A valid stateid should always be specified.  When the file 
size attribute is not set, the special stateid consisting of 
all bits zero should be passed.

Issue 4) The last paragraph of the IMPLEMENTATION section of
SETATTR, besides the practical problems in that following
the recommendation of returning INVAL (you did something wrong
and I'm not going to tell you what it is.  Nya Nya Nya), 
doesn't fit the error list that follows.  It does have things 
in it other than INVAL, which presumably would prevent you
from successfully setting the attributes.  If you can't
set the attributes because of an IO error, you return IO,
not INVAL.  Also FBIG is there.  FBIG says the error is 
extending a file beyond the server limit.  Why doesn't
extending it beyond 4 Gig when the server can't handle that 
use precisely this error, rather than INVAL. (Returning 
FBIG if the client exceeds the servers limit of 3G or 10G
but INVAL if it exceeds the server's limit of 4G is kind
of insane).

Proposal 4) Delete this paragraph.  Add the following text
to the end of the second paragraph of the IMPLEMENTATION
section.

A mask of the attibutes actually set is returned by SETATTR
in all cases.  That mask must not include attributes bits not
requested to be set by the client, and must be equal to the 
mask of attributes requested to be set only if the SETATTR 
completes without error.

Issue 5) I couldn't find the error to return if I open a file
for read and do a write.  If someone else has opened it with
WRITE-DENY, you get NFS4ERR_LOCKED (or NFS4ERR_DENIED), but what
if there is no such conflicting lock?  I can't believe that it
is intended that this go through without error.  But if not,
what is the error?  The whole apparatus of checking share locks 
at open time is predicated on open representing properly the 
operations that may be perfomred within the scope of the open.
It's true that we already violate this by allowing the special
stateid's (that I love so well), but going farther and treating
a write done under an open for read as if it were done with stateid
zero seems way too much of a bad thing.

Proposal 5) Add a new error NFS4ERR_OPENMODE:

Add to section 12 the following,

NFS4ERR_OPENMODE      The client attempted a READ, WRITE, or SETATTR
                      operation not sanctioned by the stateid passed
                      (e.g. writing to a file opend only for read).

and replace in the .x stuff,

           NFS4ERR_LOCKS_HELD      = 10037

by 

           NFS4ERR_LOCKS_HELD      = 10037,
           NFS4ERR_OPENMODE        = 10038

Add NFS4ERR_OPENMODE to the error lists for READ, WRITE, SETATTR.

Add the following paragraph as the third paragraph of section 8.1.4,

Every stateid other than the special stateid values noted above,
whether returned by an OPEN-type operation (i.e. OPEN, OPEN_DOWNGRADE),
or by a LOCK-type operation (i.e. LOCK or LOCKU), defines an access
mode for the file (i.e. READ, WRITE, or READ_WRITE) as established
by the original OPEN which began the stateid sequence, and as modified 
by subsequent OPEN's and OPEN_DOWNGRADE's within that stateid sequence.
When a READ, WRITE, or SETATTR which specifies the size attribute 
is done, if the access mode for the stateid does not allow the 
operation to be perofmred (for these purposes SETATTR which sets
the size attribute is treated like a WRITE), the operation should
fail with an NFS4ERR_OPENMODE.

Issue 6) Section 8.1.4 is a mess and the hacking suggested above 
hasn't improved its clarity.

Proposal 6) Once we come to agreement on the stuff above, I will
try to redraft section 8.1.4 so that it is clearer and more readable.