Tried to clarify when O_NAMEDATTR is used.

I am now convinced that the check in zfs_freebsd_open()
for O_NAMEDATTR being set is not necessary at all.

Checking for VIRF_NAMEDATTR in zfs_freebsd_open()
was not needed and broke the runat(1) utility I am writing.

Add a missing '.' at the end of a statement.

Oops. I didn't see emaste@'s comment until after
doing the commit.

In D49718#1134202, @ziaee wrote:

I do really love putting the commit that the work depends on in "Fixes: " instead of the introductory sentence of the commit message. It makes me feel like the the sentence is clean and just explaining for me, and very predictable to track the tree of events related to the commit.

Removed the sentence w.r.t. the specific error, since
it is covered in the ERROR section. Also, moved the
ENOATTR entries so that they are in alphabetical order,
as suggested by ziaee@.

Moved the MNT_NAMEDATTR entry up, to maintain
alphabetical ordering, as suggested by ziaee@.

Just to clarify what the previous comment was talking about.
Without this patch:
fhopen(&fh, O_RDONLY | O_NAMEDATTR | O_PATH) works
but
fhopen(&fh, O_EXEC | O_NAMEDATTR | O_PATH) fails with EINVAL
(You can basically ignore the O_NAMEDATTR. I just did the tests with
some test code I had lying about for O_NAMEDATTR.)

Without this patch:
kern_fhopen() requires FREAD or FWRITE.
It checks for these and fails with EINVAL if neither are set.

This looks fine to me.

Cleanup up zfs_freebsd_lookup(), including adding some
comments to try and explain the named attribute case.

Fixed a bug when open'ng "." in a named attribute directory.

All looks good to me. (I am glad you did this. I would have missed
half of the places that needed to be changed.)

In D49583#1131634, @kib wrote:

Do you want me to prepare the generic patch for stat(2)? Then you would add the setting of the bits as the followup.

In D49583#1131392, @kib wrote:

In D49583#1131391, @rmacklem wrote:

In D49583#1131379, @kib wrote:

I think fhopen() should require O_NAMEDATTR to successfully open the attribute inode' handle.
BTW, given a file descriptor, is there a way to understand if this is a regular fd or namedattr fd? In particular, does fstatat(2) return some distinguishing info?

Not a fd. The code needs to get the associated vnode and look for VIRF_NAMEDDIR or VIRF_NAMEDATTR being set.
(I'll tweak fhopen(2) to do that.)

No, I don't have fstatat(2) returning anything in "struct stat" that indicates "namedattr".
(st_flags seem to be all bits that can be set/cleared by chflags and I cannot see any
other field of "struct stat" that could be used to indicate a named attribute dir/attribute.)
Any suggestions?

The regular way would be to define new S_IFMT value for named attr files, but this probably would break too much.
We have st_padding0 field that can be used for additional flags, like st_bsd_flags or similar, and we can allocate the first bit there to mean named attr file. The rename of st_padding0 and the flag definition should be a separate commit.

Change fhopen(2) so that O_NAMEDATTR is required to
open a file handle that represents a named attribute or
named attribute directory.

In D49583#1131379, @kib wrote:

I think fhopen() should require O_NAMEDATTR to successfully open the attribute inode' handle.
BTW, given a file descriptor, is there a way to understand if this is a regular fd or namedattr fd? In particular, does fstatat(2) return some distinguishing info?

Changed MNTK_NAMEDATTR to MNT_NAMEDATTR as
suggested by kib@.

Fyi, I did a little testing of fhopen(2) and getfhat(2) and
everything seems to work.

W.r.t. fhopen(2), I think there is a couple of ways to handle
O_NAMEDATTR.

• Just don't allow it, similar to O_CREAT.

or

• Fail the fhopen(2) if the file handle does not represent a named attribute directory or a named attribute file.

I need to think about fhopen(2) and friends.
I thought I had disabled use of O_NAMEDATTR
for them, but maybe I missed that in the patch.

This looks fine to me and does simplify things, I think?

Looks fine to me.

Looks fine to me, rick

Looks ok to me. You can decide if retries make sense for
ecode == EAI_NONAME.

Looks fine to me.

In D48570#1112658, @glebius wrote:

In D48570#1112656, @rmacklem wrote:

Looks ok to me but I do not know pthreads.
Maybe markj@ can complete a review, since he
has already commented on the code?

If you don't mind, I'd like to push changes today. I will be online through the weekend and not disrupted by usual work & weekday stuff, so will be able to address any issues that may be discovered.

Looks ok to me but I do not know pthreads.
Maybe markj@ can complete a review, since he
has already commented on the code?

Looks ok to me but I do not know pthreads.