Changeset View
Standalone View
sys/kern/vfs_subr.c
Show First 20 Lines • Show All 5,338 Lines • ▼ Show 20 Lines | if (hint == NOTE_REVOKE || (hint == 0 && vp->v_type == VBAD)) { | ||||
VI_UNLOCK(vp); | VI_UNLOCK(vp); | ||||
return (1); | return (1); | ||||
} | } | ||||
res = (kn->kn_fflags != 0); | res = (kn->kn_fflags != 0); | ||||
VI_UNLOCK(vp); | VI_UNLOCK(vp); | ||||
return (res); | return (res); | ||||
} | } | ||||
/* | |||||
* Returns whether the directory is empty or not. | |||||
* If it is empty, the return value is 0; otherwise | |||||
* the return value is an error value (which may | |||||
* be ENOTEMPTY). | |||||
*/ | |||||
kib: Remove the empty line. | |||||
int | |||||
vfs_emptydir(struct vnode *vp, struct thread *td) | |||||
Not Done Inline ActionsI still suggest to remove the td argument. kib: I still suggest to remove the td argument. | |||||
Done Inline ActionsBut you still haven't said why. It's used -- by the uio, which admittedly may not use it, and by VOP_READDIR(), which may (at least for the thread credential). sef: But you still haven't said why. It's used -- by the uio, which admittedly may not use it, and… | |||||
Not Done Inline ActionsBecause there is high desire to drop trivial td arguments. The reason why td was used and passed around is long time gone. Now the code makes an impression that it can work with borrowed context of arbitrary thread, but it cannot. More, the special cases where it should be able to work with td != curthread is very hard to identify. For that reasons, td is slowly removed from kernel, and new functions should not take the arg unless there is some reason. kib: Because there is high desire to drop trivial td arguments.
The reason why td was used and… | |||||
Done Inline ActionsHm, ok. So I should just use curthread and curthread's cred? That strikes me as risky, but then again, I'm not sure where it wouldn't be valid, correct? sef: Hm, ok. So I should just use curthread and curthread's cred? That strikes me as risky, but… | |||||
Not Done Inline ActionsRisky in which way ? kib: Risky in which way ? | |||||
Done Inline ActionsInherent distrust of global state. As I said, I am not sure where that global state *wouldn't* be valid. (That is, I was trying to follow my logic through, and didn't see a case where relying on curthread was unsafe. So my inherent distrust is wrong, correct?) sef: Inherent distrust of global state. As I said, I am not sure where that global state *wouldn't*… | |||||
Not Done Inline Actionscurthread is always valid. Well, there are cases of early initialization and context switch where curthread handling is delicate, but it is definitely not in any code that touches vnodes or executes in the syscall context. kib: curthread is always valid.
Well, there are cases of early initialization and context switch… | |||||
{ | |||||
struct uio uio; | |||||
struct ucred *cred = td->td_ucred; | |||||
Not Done Inline ActionsThis var is not very useful. kib: This var is not very useful. | |||||
Done Inline ActionsHoldover -- I realized I could add the td argument to it, and forgot to get rid of all the vestiges from before that. sef: Holdover -- I realized I could add the td argument to it, and forgot to get rid of all the… | |||||
struct iovec iov; | |||||
struct dirent *dirent, *dp, *endp; | |||||
int error = 0; | |||||
int eof = 0; | |||||
Done Inline ActionsPut eof declaration together with error. -1 line. kib: Put eof declaration together with error. -1 line. | |||||
int dolock = 0; | |||||
kibUnsubmitted Done Inline Actionsstyle(9): no initialization in declaration. kib: style(9): no initialization in declaration. | |||||
sefAuthorUnsubmitted Done Inline Actionsstyle(9) says, "Be careful to not obfuscate the code" and "Use this feature only thoughtfully." Again, there are already instances in the file of that -- sysctl_ftry-reclaim_vnode, sched_sync, speedup_syncer. I don't see how it obfuscates the code; it makes it clear those are initial values. sef: style(9) says, "Be careful to not obfuscate the code" and "Use this feature only thoughtfully."… | |||||
dirent = malloc(sizeof(struct dirent), M_TEMP, M_WAITOK); | |||||
kibUnsubmitted Not Done Inline ActionsIt is undesirable to call malloc() under a vnode lock, because pagedaemon might need the vnode lock to clean pages. kib: It is undesirable to call malloc() under a vnode lock, because pagedaemon might need the vnode… | |||||
sefAuthorUnsubmitted Done Inline ActionsI based the code on other uses of readdir in the kernel (blech). I'm open to suggestions there! sef: I based the code on other uses of readdir in the kernel (blech). I'm open to suggestions there! | |||||
endp = (void*)((uint8_t*)dirent + sizeof(struct dirent)); | |||||
kibUnsubmitted Done Inline ActionsStyle: void * (space before start). kib: Style: `void *` (space before start). | |||||
iov.iov_base = dirent; | |||||
iov.iov_len = sizeof(struct dirent); | |||||
Done Inline Actionsendp = dirent + 1; ? kib: `endp = dirent + 1;` ? | |||||
Done Inline Actions... duh. That is so much clearer. Except, it's not needed at all, since I set endp in the for loop below before checking it. sef: ... duh. That is so much clearer.
Except, it's not needed at all, since I set endp in the for… | |||||
uio.uio_iov = &iov; | |||||
uio.uio_iovcnt = 1; | |||||
uio.uio_offset = 0; // Start at the beginning | |||||
uio.uio_resid = sizeof(struct dirent); | |||||
uio.uio_segflg = UIO_SYSSPACE; | |||||
uio.uio_rw = UIO_READ; | |||||
Done Inline ActionsStyle only allows C comments /* */. That said, the comment above is rather non-informative. kib: Style only allows C comments /* */. That said, the comment above is rather non-informative. | |||||
uio.uio_td = curthread; | |||||
if (VOP_ISLOCKED(vp) == 0) { | |||||
kibUnsubmitted Done Inline ActionsThis is a wrong test, VOP_ISLOCKED() might return LK_EXCLOTHER or LK_SHARED. But it does not matter much because vnode is asserted exclusively locked at line 863. So this block must be removed. kib: This is a wrong test, VOP_ISLOCKED() might return LK_EXCLOTHER or LK_SHARED. But it does not… | |||||
sefAuthorUnsubmitted Done Inline ActionsBut that's a different function, and I can anticipate other callers. I've changed it, but I feel concern. sef: But that's a different function, and I can anticipate other callers. I've changed it, but I… | |||||
kibUnsubmitted Not Done Inline ActionsThen add ASSERT_VOP_LOCKED(). It is redundant because pre-condition for VOP_READDIR() asserts that anyway. kib: Then add ASSERT_VOP_LOCKED(). It is redundant because pre-condition for VOP_READDIR() asserts… | |||||
Done Inline ActionsWhy not td ? But I strongly suggest to remove the td arg at all, it is pointless. kib: Why not td ? But I strongly suggest to remove the td arg at all, it is pointless. | |||||
Done Inline ActionsAgain, holdover from before I added the td argument. sef: Again, holdover from before I added the td argument. | |||||
dolock = 1; | |||||
Done Inline ActionsExtra blank line. kib: Extra blank line. | |||||
vn_lock(vp, LK_SHARED | LK_RETRY); | |||||
} | |||||
while (eof == 0) { | |||||
error = VOP_READDIR(vp, &uio, cred, &eof, NULL, NULL); | |||||
if (error == 0) { | |||||
kibUnsubmitted Done Inline Actions`if (error != 0) break; ` kib: `if (error != 0)
break;
`
then unindent the rest of the loop body. | |||||
endp = (void*)((uint8_t*)dirent + | |||||
Done Inline ActionsExtra (). kib: Extra (). | |||||
(sizeof(struct dirent) - uio.uio_resid)); | |||||
for (dp = dirent; | |||||
dp < endp; | |||||
kibUnsubmitted Done Inline ActionsI do not see why do you need new line there. kib: I do not see why do you need new line there. | |||||
sefAuthorUnsubmitted Done Inline ActionsIndentation and 80 columns. No longer indented an extra layer, so I joined a couple of the lines together, and it looks better. sef: Indentation and 80 columns. No longer indented an extra layer, so I joined a couple of the… | |||||
dp = (void*)((uint8_t*)dp + | |||||
GENERIC_DIRSIZ(dp))) { | |||||
if (dp->d_type == DT_WHT) | |||||
kibUnsubmitted Not Done Inline ActionsWhy whiteout is not qualified for non-emptiness ? I do not argue, but want to understand the cause of the decision. kib: Why whiteout is not qualified for non-emptiness ? I do not argue, but want to understand the… | |||||
sefAuthorUnsubmitted Done Inline ActionsA white-out entry isn't *there*, at that level anyway, so it looks like an empty directory. I wasn't entirely sure about that, but that was the logic. sef: A white-out entry isn't *there*, at that level anyway, so it looks like an empty directory. I… | |||||
continue; | |||||
if (dp->d_namlen == 0) | |||||
continue; | |||||
if (dp->d_type != DT_DIR && | |||||
dp->d_type != DT_UNKNOWN) { | |||||
error = ENOTEMPTY; | |||||
goto done; | |||||
kibUnsubmitted Done Inline ActionsIf you initialize error to zero before starting the loop, wouldn't bare 'break' work there ? You can remove 'done' label and gotos then. kib: If you initialize error to zero before starting the loop, wouldn't bare 'break' work there ? | |||||
sefAuthorUnsubmitted Done Inline ActionsNo, it's breaking out of two levels. Hm. Now, I *could* change it to do a "if error != 0 break" after the for loop, and I think that does what I want. (Or, as I just changed it, change the while loop to have eof ==0 and error == 0.) I _think_ I had the "error = 0;" statement because of how I was thinking about flow while writing the code, but I don't think I need it -- if the VOP_READDIR gets an error, then it can propagate up, and I think that's the correct behaviour. Please feel free to check my logic. sef: No, it's breaking out of two levels. Hm. Now, I *could* change it to do a "if error != 0… | |||||
} | |||||
if (dp->d_namlen > 2) { | |||||
error = ENOTEMPTY; | |||||
goto done; | |||||
} | |||||
if (dp->d_namlen == 1 && | |||||
dp->d_name[0] != '.') { | |||||
error = ENOTEMPTY; | |||||
goto done; | |||||
} | |||||
if (dp->d_namlen == 2 && | |||||
dp->d_name[1] != '.') { | |||||
kibUnsubmitted Not Done Inline ActionsI would list all three conditions in one if separated by '||', this saves ten lines. kib: I would list all three conditions in one if separated by '||', this saves ten lines. | |||||
sefAuthorUnsubmitted Done Inline ActionsSame format is in UFS dirempty function. (Although it doesn't do whiteout checks there, because it's checking due to rmdir.) sef: Same format is in UFS dirempty function. (Although it doesn't do whiteout checks there… | |||||
error = ENOTEMPTY; | |||||
goto done; | |||||
} | |||||
} | |||||
uio.uio_resid = sizeof(struct dirent); | |||||
} else { | |||||
printf("%s(%d): Error = %d\n", __FUNCTION__, __LINE__, error); | |||||
kibUnsubmitted Done Inline ActionsThis line should not be committed. kib: This line should not be committed. | |||||
break; | |||||
} | |||||
} | |||||
/* If we got here, there were no other entries */ | |||||
error = 0; | |||||
done: | |||||
if (dirent) | |||||
kibUnsubmitted Done Inline ActionsThe check is not needed. kib: The check is not needed. | |||||
sefAuthorUnsubmitted Done Inline ActionsHoldover from xnu days where malloc with M_WAITOK could still return NULL. sef: Holdover from xnu days where malloc with M_WAITOK could still return NULL. | |||||
free(dirent, M_TEMP); | |||||
if (dolock) | |||||
VOP_UNLOCK(vp, LK_SHARED | LK_RETRY); | |||||
return (error); | |||||
} | |||||
int | int | ||||
vfs_read_dirent(struct vop_readdir_args *ap, struct dirent *dp, off_t off) | vfs_read_dirent(struct vop_readdir_args *ap, struct dirent *dp, off_t off) | ||||
{ | { | ||||
int error; | int error; | ||||
if (dp->d_reclen > ap->a_uio->uio_resid) | if (dp->d_reclen > ap->a_uio->uio_resid) | ||||
return (ENAMETOOLONG); | return (ENAMETOOLONG); | ||||
error = uiomove(dp, dp->d_reclen, ap->a_uio); | error = uiomove(dp, dp->d_reclen, ap->a_uio); | ||||
▲ Show 20 Lines • Show All 367 Lines • Show Last 20 Lines |
Remove the empty line.