Yes, I mentioned that in Description of the review. It will make the code more complex and possibly without real usage of this. The common approach is:
1. READDIR request with rdattr_error
2. Reply contains NFS4ERR_MOVED for referrals
3. A client analyses and send GETATTR with fh_location for those previously returned entries with NFS4ERR_MOVED.

The intension of this patch - strict READDIR reply according to RFC because vmware vSphere crashes due to this.
If someone requested fh_location handling w/o set rdattr_error, then it can be added of course.

Ah, now I realized that you stated this in the review Description:

The patch intensionally misses first part of 8.3.2: returning success if fs_locations is set.


Unfortunately, this creates a regression, which is not acceptable.  Sorry.

Ha, we replied concurrently without seeing each other, and you were faster :-).
Anyway, sorry, but you cannot fix one RFC 7530 violation by creating another RFC 7530 violation, even you believe such a case is rare.
And no, you do not achieve strict READDIR reply according to RFC because you just introduces the RFC 7530 violation in a case the fs_location is requested.

Sorry, bad wording here.  I wanted to say: (and the caller of this function is not prepared to see *bmval as zero after the return from this function).

Ah, I think I understand what caused the confusion.
My interpretation of the RFC is that for the GETATTR operation for an object in the absent file system the NFS server should return fs_locations, fsid, mounted_on_fileid only and nothing else (from RFC: Other attributes SHOULD NOT be made available for absent file systems, even when it is possible to provide them.).  This is from Section 8.3.1 and it is valid for the GETATTR operation, not for READDIR.
For the READDIR operation Section 8.3.2 applies.  Here, the set of attributes is not limited.  It means that the NFS server should provide as much as possible attributes as it can.  It could easily happen that the set of the attributes that the NFS server can support will be limited to rdattr_error, fs_locations, fsid, and mounted_on_fileid only (or even without mounted_on_fileid, because it is RECOMMENDED only; and even without fsid because it could be not possible for the NFS server to determine it in some extreme case), but this limitation is not because of the Section 8.3.1.
So, you can limit the set of the attributes for the absent file system for the READDIR operation to rdattr_error, fs_locations, fsid, and mounted_on_fileid only (if we really cannot support more), but you cannot say it is because of Section 8.3.1.

I probably miss something, but I see no way how *bmval could be zero after this line.  Please show me the scenario how it can happen.  Thanks.

IOW, does it mean that this code was not needed even without your change?

Could you please use the "Reply" button so the discussion is properly threaded and we do not lost the context?  Thanks.
Sorry, but I disagree.  8.3.2 is not directly related to 8.3.1.  Section 8.3.1 applies to the GETATTR operation only.  For READDIR you should return as much attributes as possible, even the file system entry is a root of the absent filesystem.

RFC7530 doesn't have description for valid attributes for Absent Fs and only one place is 8.3.1.
Originally in Linux sources it has comment: "As per referral draft".
This document  (draft-ietf-nfsv4-referrals-00) has "3.3.  Attributes Needed for Root of an Absent Fs" and "3.4.  Attributes Valid for Root of an Absent Fs"

Sorry, but both Linux sources and RFC drafts are not authoritative sources for the NFSv4 specification.  Only RFC 7530 and other related RFCs are.  If you think the RFC is not clear enough in this area (I don't think so) you can clarify this with the IETF NFSv4 working group and/or eventually submit an RFC errata.

If I change comment with "RFC 7530 Section 8.3", will it be OK for you ?

No.  The Section 8.3 of the RFC does not limit the attributes to those four you are listing here for the READDIR operation with the absent fs entries.

So you want I remove this comment?

I would like to see implemented as much as possible attributes for READDIR for absent fs to follow RFC 7530.  In a case we need to limit the list of attributes for some reason then the reason should be well understood.

Other attributes are not valid for an absent filesystem. If you argee with that I put this information in comment for the ABSENT_FS_ATTRS.

I'm not sure it is true.  For example I think we could provide the lease_time attribute.  Probably the filehandle attribute should be supported for READDIR too.

You are wrong about filehandle. filehandle is never supported for absent filesystem and can't be returned.
lease_time is per-server attribute and returning it is also wrong asumption because client do not need it for this entry.
Additionally repeat that getting attribute via GETATTR and READDIR almost the same except READDIR is stronger and have some restriction. So that READDIR must never return more attribute than GETATTR and therefor Section 8.3.1 and 8.3.2 are actual.

filehandle is never supported for absent filesystem and can't be returned.

Such filehandle can't be returned by GETFH.  It is a consequence of the fact that if the current filehandle on a start of an operation is for an absent filesystem the operation will fail with NFS4ERR_MOVED.  But it is easily possible that the client can have the filehandle of the absent object and supply it to the server via the PUTFH operation.  Maybe one of the ways how to get the absent filehandle is the READDIR operation.



lease_time is per-server attribute and returning it is also wrong asumption because client do not need it for this entry.

We do not know what a client needs or does not need.  The important question here is:  What should happen if the client will request the lease_time attribute for the READDIR operation and there are some absent entries?  Should it be returned or not?  If not, then why not?



So that READDIR must never return more attribute than GETATTR ...

The GETATTR operation should return fs_locations, fsid, and mounted_on_fileid only (Section 8.3.1 of RFC 7530).  If "READDIR must never return more attribute than GETATTR" (your words) how is it possible that READDIR should return rdattr_error too (Section 8.3.2 of RFC 7530)?

|Such filehandle can't be returned by GETFH.  It is a consequence of the fact that if the current filehandle on a start of an

|operation is for an absent filesystem the operation will fail with NFS4ERR_MOVED.  But it is easily possible that the client can

|have the filehandle of the absent object and supply it to the server via the PUTFH operation.  Maybe one of the ways how to get

|the absent filehandle is the READDIR operation.
Your scheme via READDIR is impossible. Archivement can be done only via "LOOKUP".
| We do not know what a client needs or does not need.  The important question here is:  What should happen if the client will

| request the lease_time attribute for the READDIR operation and there are some absent entries?  Should it be returned or not?  If | not, then why not?
I don't think that current nfs4 server implementation must be more NFSv4 than Linux NFSv4 server implementation. There are lot of reasons for that. Generally, lease_time is obtained at initial stage when client gets all  server and channel information.
Restriction ABSENT_FS_ATTRS is more than enough for clients and is bigger than current illumos nfs server has.

|The GETATTR operation should return fs_locations, fsid, and mounted_on_fileid only (Section 8.3.1 of RFC 7530).  If "READDIR must

|never return more attribute than GETATTR" (your words) how is it possible that READDIR should return rdattr_error too (Section

|8.3.2 of RFC 7530)?
It is because rdattr_error generally is not attribute and is alias for GETATTR::status. READDIR reply doesn't have statuses for entries and rdattr_error is used to indicate status of getting attributes for an entry (if requested). GETTATR has status field that indicates success or fail for getting attributes.
Generally attributes must be the same, exception are in 8.3.2.

It is because rdattr_error generally is not attribute and is alias for GETATTR::status.

Please read Section 5.6 of RFC 7530 to see that rdattr_error is actually a REQUIRED attribute.
Apparently this discussion goes nowhere so I'm giving up.

It is because rdattr_error generally is not attribute and is alias for GETATTR::status.

Please read Section 5.6 of RFC 7530 to see that rdattr_error is actually a REQUIRED attribute.



I just explained origin of rdattr_error requirements in READDIR operation.




Apparently this discussion goes nowhere so I'm giving up.

I don't get what is reason to spend time for disscussion whether ABSENT_FS_ATTRS need something additional or not. I pointed to Section 8.3 in RFC7530 where it is specified.
Will update the patch according to your remarks.
Thanks.

???
Please show me how *bmval could be zero.  Thanks.

Sorry, maybe I miss something, but you asked whether is problem if bmval becomes zero after "&=". I answered no. If you ask about is it possible that bmval can be zero after "&=", then asnwer is "no" at least FS_LOCATIONS is left.
Your pointing to the caller brought to me idea to fix usage "ar" after nfs4_readdir_getvp() call. Do you approve that idea?

We do not care what other communities does :-).

If rule is not specified implicitly, I would use direct type specification due to it shows and helps to understand origin of the used variable. Additionally this practice is useful if "fs" is changed to pointer.

I do not  agree. Look at:

 "The set of attributes returned for the root

  directory of the absent file system in that case is simply

  restricted to those actually available".
That means we can limit number of attributes for an absent filesystem and ... do it as 8.3.2 says!
To satisfy your complaints I would add the following to ABSENT_FS_ATTRS description:
/* 
 * RFC 7530 Section 8.3 
 * Supported number of attributes for the root of an absent filesystem. 
 */

Yes, exactly.  The only problem is that 8.3.2 does not say what should be the set of available attributes.
We have two cases here:
1) GETATTR - in this case the set of supported attributes is: fs_locations, fsid, mounted_on_fileid (and nothing else) - see 8.3.1.

2) READDIR - in this case the set of the supported attributes could (and very likely will) be limited, but the RFC does not specify the exact list (see 8.3.2).  We should support as much attributes as possible.
The question is: what is the right list of attributes we can support for the READDIR case?  It is easily possible that the list contains those 4 attributes only (those you defined as ABSENT_FS_ATTRS).  We can do that, but the limit to those 4 attributes is not because the RFC says so, but because we have no way how to support more attributes.

I suggest to leave it as is, because the polishing doesn't move a train forward. It would be nice if RFC7530 had full description for all supported attributes for READDIR in case of an absent filesystem similar for GETATTR.

It is correct, because rndup is measured in BYTES_PER_XDR_UNIT == 4 bytes.
Additionally want to note that it is not my change: It is not my change:  ^7c478bd953 (stevel@tonic-gate  2005-06-14  784)

we should not need void * casts for pointer (for second argument);

Why do you think so? Pointers for deduction should point to the same type, otherwise it is not compiled. Try "(void *)ptr_redzone - ptr" as you suggested.




uintptr_t to do the arithmetics and then cast final result to void *

It can be done in any variants, so I would leave it as is, but if you insist and in illumos kernel style is using uintptr_t, I will replace it with uintptr_t.

I do not mind either way as long as it is working:D Sure we can play the game of casts, but excessive casting is quite unreadable too.

You are right about style of this block of code. But this patch fixes 'reference issue' #7782 and doesn't refactor all generic code (intention is stability, no regression and blame-annotate). Let's leave it as is.
If you have checker tool's warning it should be fixed of course.

The same as for previous: Let's leave generic code as is  and really "goto reencode_attrs" is informative in this case.
If you have checker tool's warning I will fix it.

Tools can't check for misused gotos, but using goto like these doesn't conform to the C Style and Coding Standards.

From cstyle(1ONBLD) manual page:


...

It attempts to check for the cstyle documented in cstyle.ms.pdf.

Note that there is much in that document that cannot be checked for;

just because your code is cstyle(1ONBLD) clean does not mean that you've

followed illumos C style. Caveat emptor.

Please create separate ticket for fixing these instead of leaving as-is

(of course, you can leave this style clean-up job to somebody else).
A comment before the if (ar != 0) { line could be better to describe why this decision is made

and what this section of code will do in summary.

Issue #9670 created.
Thanks! Created separate ticket as fixing those are out of current ticket description.

Issue #9670 created.

Issue #9670 created.