On Sat, Jul 27, 2024 at 08:07:12AM +1000, NeilBrown wrote:
> On Sat, 27 Jul 2024, Chuck Lever wrote:
> > On Thu, Jul 25, 2024 at 10:21:32PM -0400, NeilBrown wrote:
> > > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> > > index a7a07470c1f8..8d75759c600d 100644
> > > --- a/fs/nfsd/nfs3xdr.c
> > > +++ b/fs/nfsd/nfs3xdr.c
> > > @@ -111,6 +111,23 @@ svcxdr_encode_nfsstat3(struct xdr_stream *xdr, __be32 status)
> > > {
> > > __be32 *p;
> > >
> > > + switch (status) {
> > > + case nfserr_symlink_not_dir:
> > > + status = nfserr_notdir;
> > > + break;
> > > + case nfserr_symlink:
> > > + case nfserr_wrong_type:
> > > + status = nfserr_inval;
> > > + break;
> > > + case nfserr_nofilehandle:
> > > + status = nfserr_badhandle;
> > > + break;
> > > + case nfserr_wrongsec:
> > > + case nfserr_file_open:
> > > + status = nfserr_acces;
> > > + break;
> > > + }
> > > +
> >
> > This is entirely the wrong place for this logic. It is specific to
> > symlinks, and it is not XDR layer functionality. I prefer to see
> > this moved to the proc functions that need it.
>
> The logic in not specific to symlinks. It is specific to RPC procedures
> which act on things that are not expected to be symlinks. There are
> lots of procedures like that.
Fair enough.
IMO, it will help our review to see exactly which procedures are
impacted. That suggests to me that moving the impact into specific
procedures will also make the design less brittle overall, but
that's just intuition at this point.
> We could put the mapping in each .pc_func function, or between the
> proc->pc_func() call in nfsd_dispatch() and the following
> proc->pc_encode() call, or in each .pc_encode call.
>
> Putting it in each .pc_func would mean changing the "return status;" at
> the end of each nfs3_proc_* function to "return map_status(status);"
>
> Putting it between ->pc_func() and ->pc_encode() would mean defining a
> new ->pc_map_status() for each program/version and calling that.
>
> Putting it in each .pc_encode is what I did.
>
> It isn't clear to me that either of the first two are better than the
> third, but neither are terrible. However the second wouldn't work for
> NFSv4.0 mapping (as individual ops need to be mapped). But v4.0 needs
> special handling anyway.
>
> Where would you prefer I put it?
My thought was to put the status code mapping in the proc->pc_func
calls themselves. It sounds like that will work for NFSv4 too.
If this arrangement is too ugly for words we can rethink.
> > The layering here is that XDR should handle only data serialization.
> > Transformations like this go upstairs in the proc layer. I know NFSD
> > doesn't always adhere to this layering model, very often out of
> > convenience, but I want new code to try to stick to a stricter
> > layering model.
> >
> > Again, the reason for this is that eventually much of the existing
> > XDR code will be replaced by machine-generated code. Adding bespoke
> > bits of logic here makes the task of converting this code more
> > difficult.
>
> I suspect it we won't be able to avoid the machine generated code
> occasionally calling bespoke code - but we won't know until we try.
I think there are lots of simple encode/decode functions that will
work fine immediately with generated code. There are some areas
that will need plenty of massage. There are some spots that won't
work at all. I'm not planning for perfect coverage ;-)
(And, like a duck, I mention this plan every so often while gliding
along smoothly, but my little webbed feet are paddling away
furiously under the water's surface working on this project. Stay
tuned.)
> > It's confusing to me that we're using the same naming scheme for
> > symbolic status codes defined by spec and the ones defined for
> > internal use only. It would be nice to rename these, say, with an
> > _INT_ or _INTL_ in their name somewhere.
>
> Well the spec say v4 statuses start NFS4ERR_ but we don't follow that.
> It isn't clear to me that disambiguation the name would help. At the
> point where the error is returned the important thing is what the error
> means, that that is spelt out in the text after nfserr_*. At the point
> where the error is interpreted the meaning is obvious in the code.
> From the perspective of NFSv2, nfserr_xdev is an internal error code.
> From the perspective of NFSv3 it is not. Should it have 'int'?]
>
> But if you really want this and you can tell me exactly where you want
> the INT or INTL (and presumably int or intl for the be32 version) I'll do it.
Points taken. Let's postpone renaming for now. If the symbol naming
scheme is still vexing me in a few weeks, I will write a patch for
it.
> > A short comment that explains why these /internal/ error codes need
> > big-endian versions would be helpful to add. I assume it's because
> > they will be stored or returned via a __be32 result that actually
> > does sometimes carry an on-the-wire status code.
>
> Yes exactly. I'll add some text like that.
Thanks for your feedback.
--
Chuck Lever
