On Tue, 13 Aug 2024, Chuck Lever wrote: > On Mon, Aug 12, 2024 at 02:06:25PM +1000, NeilBrown wrote: > > > > [This should be squashed into the existing patch for the same name, > > with this commit message used instead of the current one. It addresses > > the pynfs failures that Jeff found] > > > > If an NFS operation expects a particular sort of object (file, dir, link, > > etc) but gets a file handle for a different sort of object, it must > > return an error. The actual error varies among NFS version in non-trivial > > ways. > > > > For v2 and v3 there are ISDIR and NOTDIR errors and, for NFSv4 only, > > INVAL is suitable. > > > > For v4.0 there is also NFS4ERR_SYMLINK which should be used if a SYMLINK > > was found when not expected. This take precedence over NOTDIR. > > > > For v4.1+ there is also NFS4ERR_WRONG_TYPE which should be used in > > preference to EINVAL when none of the specific error codes apply. > > > > When nfsd_mode_check() finds a symlink where it expected a directory it > > needs to return an error code that can be converted to NOTDIR for v2 or > > v3 but will be SYMLINK for v4. It must be different from the error > > code returns when it finds a symlink but expects a regular file - that > > must be converted to EINVAL or SYMLINK. > > > > So we introduce an internal error code nfserr_symlink_not_dir which each > > version converts as appropriate. > > > > nfsd_check_obj_isreg() is similar to nfsd_mode_check() except that it is > > only used by NFSv4 and only for OPEN. NFSERR_INVAL is never a suitable > > error if the object is the wrong time. For v4.0 we use nfserr_symlink > > for non-dirs even if not a symlink. For v4.1 we have nfserr_wrong_type. > > To handle this difference we introduce an internal status code > > nfserr_wrong_type_open. > > > > As a result of these changes, nfsd_mode_check() doesn't need an rqstp > > arg any more. > > > > Note that NFSv4 operations are actually performed in the xdr code(!!!) > > so to the only place that we can map the status code successfully is in > > nfsd4_encode_operation(). > > Thanks for noting the RFC sections in the comments, that will be > very helpful for us forgetful old farts. > > nfsd_check_obj_isreg's only caller is do_open_lookup, and the minor > version number is available there, via nfsd4_compound_state. If the > minor version is passed to nfsd_check_obj_isreg, it can correctly > choose to return nfserr_symlink or nfserr_wrong_type, yes? That > would eliminate the need for nfserr_wrong_type_open, perhaps. Probably a good idea. The original goal of this series was to reduce the number of checks on version number by moving version-specific choices to version-specific code. However that doesn't apply to differences between 4.0 and 4.1 as we don't have distinct code for the distinct minor versions. I'll post an alternate SQUASH patch. > > What were the other error code leaks you found? They were happening because I was calling nfsd4_map_status() before nfsd4_encode_operation(). This is *before* the status in set in many cases. status is often set inside the xdr encode functions called by nfsd4_encode_operation(). Moving the call to nfsd4_map_status() inside that function fixed those leaks. NeilBrown
