getdirentries: Return ENOTDIR if not a directory.

[des]

Summary

This is both more logical and more useful than EINVAL.

MFC after: never
Sponsored by: Klara, Inc.

[kib]

lib/libsys/getdirentries.2

173

I do not object against the change.
Just a note that with it, sometimes this condition would be reported as ENOTDIR now.
Specifically, when e.g. the mount where the corresponding vnode is located, is forcibly unmounted.

[des]

fix test

[des]

lib/libsys/getdirentries.2

173

Sorry, I don't understand what this means, or how it relates to the line it's attached to.

[des]

improve tests

[kib]

lib/libsys/getdirentries.2

173

The EBADF condition that is described in the enumeration item I commented, after the patch might some time (most of the time in fact) result in ENOTDIR. But some times it is still EBADF.

[des]

lib/libsys/getdirentries.2

173

It would previously have returned EINVAL, never EBADF, but I've fixed it now.

[des]

return EBADF for VBAD.

[kib]

lib/libsys/getdirentries.2

173

Sometimes it was still EBADF when falling to deadfs VOP.

sys/kern/vfs_syscalls.c

4317

This is racy, the vnode type can change from V* to VBAD any time when the vnode lock is not owned.

At least, move the check after the vn_lock. And please add a comment explaining why we check the vnode type at all, since usually we either not do it at all, or do vn_lock() without LK_RETRY and check vn_lock() failures.

[des]

sys/kern/vfs_syscalls.c

4317

In that case we may as well drop the check and let the call through to the VOP, which will return either EBADF or ENOTDIR anyway, no?

[kib]

sys/kern/vfs_syscalls.c

4317

Look at ufs_readdir(): it does not tread directories specially, it reads the data pages and interpret the content as dirents. In other words, I think that removing the check cannot be done with the current code, and would require passing over all fs ufs_readdir() to ensure ENOTDIR.

[des]

sys/kern/vfs_syscalls.c

4317

Is it safe to assume that VNON can't happen?

[des]

check while locked

[kib]

sys/kern/vfs_syscalls.c

4317

Cannot happen in what sense?

If you have a directory vnode, and it is not locked, it can be reclaimed at any moment, at which time it is converted to VBAD. So if you checked the type of a vnode and it is not VDIR, the vnode would not magically become VDIR later (we do not expose half-constructed vnodes).

[des]

sys/kern/vfs_syscalls.c

4317

I'm asking if it's safe to assume that the vnode's type must either be valid or VBAD, and can never be VNON at this point, so I don't need to check for it. I think it's safe, but I'd still like you to confirm it.

[kib]

sys/kern/vfs_syscalls.c

4317

I think I answered it in the second paragraph of my previous response.

Also, I do not see why would you need to specifically check for VNON. If you have a non-directory type vnode, the only special case is the doomed vnode where it is better to fine-select EBADF.

[des]

sys/kern/vfs_syscalls.c

4317

Because imo VNON would also be an EBADF case rather than ENOTDIR. But it can't happen, so it doesn't matter.