This is both more logical and more useful than EINVAL.
MFC after: never
Sponsored by: Klara, Inc.
Details
Details
- Reviewers
kib jhb kevans - Group Reviewers
Klara - Commits
- rGdd81cc2bc5f2: getdirentries: Return ENOTDIR if not a directory.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 65303 Build 62186: arc lint + arc unit
Event Timeline
| lib/libsys/getdirentries.2 | ||
|---|---|---|
| 173 | I do not object against the change. | |
| lib/libsys/getdirentries.2 | ||
|---|---|---|
| 173 | Sorry, I don't understand what this means, or how it relates to the line it's attached to. | |
| 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. | |
| lib/libsys/getdirentries.2 | ||
|---|---|---|
| 173 | It would previously have returned EINVAL, never EBADF, but I've fixed it now. | |
| 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. | |
| 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? | |
| 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. | |
| sys/kern/vfs_syscalls.c | ||
|---|---|---|
| 4317 | Is it safe to assume that VNON can't happen? | |
| 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). | |
| 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. | |
| 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. | |
| 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. | |