Changeset View
Standalone View
sys/kern/vfs_syscalls.c
Show First 20 Lines • Show All 99 Lines • ▼ Show 20 Lines | |||||
static int setfflags(struct thread *td, struct vnode *, u_long); | static int setfflags(struct thread *td, struct vnode *, u_long); | ||||
static int getutimes(const struct timeval *, enum uio_seg, struct timespec *); | static int getutimes(const struct timeval *, enum uio_seg, struct timespec *); | ||||
static int getutimens(const struct timespec *, enum uio_seg, | static int getutimens(const struct timespec *, enum uio_seg, | ||||
struct timespec *, int *); | struct timespec *, int *); | ||||
static int setutimes(struct thread *td, struct vnode *, | static int setutimes(struct thread *td, struct vnode *, | ||||
const struct timespec *, int, int); | const struct timespec *, int, int); | ||||
static int vn_access(struct vnode *vp, int user_flags, struct ucred *cred, | static int vn_access(struct vnode *vp, int user_flags, struct ucred *cred, | ||||
struct thread *td); | struct thread *td); | ||||
static int kern_fhlinkat(struct thread *td, int fd, const char *path, | |||||
enum uio_seg pathseg, fhandle_t *fhp); | |||||
static int kern_getfhat(struct thread *td, int flags, int fd, | |||||
const char *path, enum uio_seg pathseg, fhandle_t *fhp); | |||||
static int kern_readlink_vp(struct vnode *vp, char *buf, enum uio_seg bufseg, | |||||
size_t count, struct thread *td); | |||||
kib: Wrong indent for continuation line. | |||||
static int kern_linkat_vp(struct thread *td, struct vnode *vp, int fd, | |||||
const char *path, enum uio_seg segflag); | |||||
Not Done Inline ActionsThere too. kib: There too. | |||||
/* | /* | ||||
* Sync each mounted filesystem. | * Sync each mounted filesystem. | ||||
*/ | */ | ||||
#ifndef _SYS_SYSPROTO_H_ | #ifndef _SYS_SYSPROTO_H_ | ||||
struct sync_args { | struct sync_args { | ||||
int dummy; | int dummy; | ||||
}; | }; | ||||
▲ Show 20 Lines • Show All 1,371 Lines • ▼ Show 20 Lines | if (error != 0) | ||||
return (error); | return (error); | ||||
} | } | ||||
return (0); | return (0); | ||||
} | } | ||||
int | int | ||||
kern_linkat(struct thread *td, int fd1, int fd2, const char *path1, | kern_linkat(struct thread *td, int fd1, int fd2, const char *path1, | ||||
const char *path2, enum uio_seg segflg, int follow) | const char *path2, enum uio_seg segflag, int follow) | ||||
{ | { | ||||
struct vnode *vp; | struct vnode *vp; | ||||
struct mount *mp; | |||||
struct nameidata nd; | struct nameidata nd; | ||||
int error; | int error; | ||||
again: | do { | ||||
bwillwrite(); | bwillwrite(); | ||||
NDINIT_ATRIGHTS(&nd, LOOKUP, follow | AUDITVNODE1, segflg, path1, fd1, | NDINIT_ATRIGHTS(&nd, LOOKUP, follow | AUDITVNODE1, segflag, path1, fd1, | ||||
&cap_linkat_source_rights, td); | &cap_linkat_source_rights, td); | ||||
if ((error = namei(&nd)) != 0) | if ((error = namei(&nd)) != 0) | ||||
Not Done Inline ActionsBlank line is not needed. kib: Blank line is not needed. | |||||
return (error); | return (error); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
vp = nd.ni_vp; | vp = nd.ni_vp; | ||||
} while ((error = kern_linkat_vp(td, vp, fd2, path2, segflag) == EAGAIN)); | |||||
return (error); | |||||
Not Done Inline ActionsBlank line is not needed. kib: Blank line is not needed. | |||||
} | |||||
static int | |||||
kern_linkat_vp(struct thread *td, struct vnode *vp, int fd, const char *path, | |||||
enum uio_seg segflag) | |||||
{ | |||||
Not Done Inline ActionsSee the comment about vn_XXX namespace. kib: See the comment about vn_XXX namespace. | |||||
struct nameidata nd; | |||||
Not Done Inline ActionsWrong indentation for the continuation line kib: Wrong indentation for the continuation line | |||||
struct mount *mp; | |||||
int error; | |||||
if (vp->v_type == VDIR) { | if (vp->v_type == VDIR) { | ||||
vrele(vp); | vrele(vp); | ||||
return (EPERM); /* POSIX */ | return (EPERM); /* POSIX */ | ||||
} | } | ||||
NDINIT_ATRIGHTS(&nd, CREATE, | NDINIT_ATRIGHTS(&nd, CREATE, | ||||
LOCKPARENT | SAVENAME | AUDITVNODE2 | NOCACHE, segflg, path2, fd2, | LOCKPARENT | SAVENAME | AUDITVNODE2 | NOCACHE, segflag, path, fd, | ||||
&cap_linkat_target_rights, td); | &cap_linkat_target_rights, td); | ||||
if ((error = namei(&nd)) == 0) { | if ((error = namei(&nd)) == 0) { | ||||
Not Done Inline ActionsThis call locks vnodes internally. Your use of vn_linkat() while the vp is locked can cause lock recursion at least. Unfortunately WITNESS cannot detect the issues because vnode locks have same names. kib: This call locks vnodes internally. Your use of vn_linkat() while the vp is locked can cause… | |||||
Done Inline Actionswe perform a vput() before the first EAGAIN, the second EAGAIN will never be reached because the locked case will match the previous else is. so all shold be fine? maybe i could add a comment jack_gandi.net: we perform a `vput()` before the first EAGAIN, the second EAGAIN will never be reached because… | |||||
Not Done Inline ActionsI do not see how can you claim that there is no longer the lock order/lock recursion issue. namei() locks the ni_vp vnode internally, and if LOCK_LEAF flag is not provided, it unlocks the vnode before returning. You simply cannot call namei() while any vnode is locked, it is very basic rule of our VFS. kib: I do not see how can you claim that there is no longer the lock order/lock recursion issue. | |||||
Done Inline Actionslinkat doesn't LOCK_LEAF on the source node either so I've just added a VOP_UNLOCK as I haven't found a way to get VFS_FHTOVP to unlock before returning, the documenation is outdated and doesn't even show the flags argument to the macro. jack_gandi.net: `linkat` doesn't LOCK_LEAF on the source node either so I've just added a `VOP_UNLOCK` as I… | |||||
if (nd.ni_vp != NULL) { | if (nd.ni_vp != NULL) { | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
if (nd.ni_dvp == nd.ni_vp) | if (nd.ni_dvp == nd.ni_vp) | ||||
vrele(nd.ni_dvp); | vrele(nd.ni_dvp); | ||||
else | else | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
vrele(nd.ni_vp); | vrele(nd.ni_vp); | ||||
vrele(vp); | vrele(vp); | ||||
Show All 25 Lines | #endif | ||||
if (error != 0) { | if (error != 0) { | ||||
vput(vp); | vput(vp); | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
error = vn_start_write(NULL, &mp, | error = vn_start_write(NULL, &mp, | ||||
V_XSLEEP | PCATCH); | V_XSLEEP | PCATCH); | ||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
goto again; | return (EAGAIN); | ||||
} | } | ||||
error = VOP_LINK(nd.ni_dvp, vp, &nd.ni_cnd); | error = VOP_LINK(nd.ni_dvp, vp, &nd.ni_cnd); | ||||
VOP_UNLOCK(vp, 0); | VOP_UNLOCK(vp, 0); | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
vn_finished_write(mp); | vn_finished_write(mp); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
} else { | } else { | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
vrele(vp); | vrele(vp); | ||||
goto again; | return (EAGAIN); | ||||
} | } | ||||
} | } | ||||
vrele(vp); | vrele(vp); | ||||
return (error); | return (error); | ||||
} | } | ||||
/* | /* | ||||
* Make a symbolic link. | * Make a symbolic link. | ||||
▲ Show 20 Lines • Show All 901 Lines • ▼ Show 20 Lines | return (kern_readlinkat(td, uap->fd, uap->path, UIO_USERSPACE, | ||||
uap->buf, UIO_USERSPACE, uap->bufsize)); | uap->buf, UIO_USERSPACE, uap->bufsize)); | ||||
} | } | ||||
int | int | ||||
kern_readlinkat(struct thread *td, int fd, const char *path, | kern_readlinkat(struct thread *td, int fd, const char *path, | ||||
enum uio_seg pathseg, char *buf, enum uio_seg bufseg, size_t count) | enum uio_seg pathseg, char *buf, enum uio_seg bufseg, size_t count) | ||||
{ | { | ||||
struct vnode *vp; | struct vnode *vp; | ||||
struct iovec aiov; | |||||
struct uio auio; | |||||
struct nameidata nd; | struct nameidata nd; | ||||
int error; | int error; | ||||
if (count > IOSIZE_MAX) | if (count > IOSIZE_MAX) | ||||
return (EINVAL); | return (EINVAL); | ||||
NDINIT_AT(&nd, LOOKUP, NOFOLLOW | LOCKSHARED | LOCKLEAF | AUDITVNODE1, | NDINIT_AT(&nd, LOOKUP, NOFOLLOW | LOCKSHARED | LOCKLEAF | AUDITVNODE1, | ||||
pathseg, path, fd, td); | pathseg, path, fd, td); | ||||
if ((error = namei(&nd)) != 0) | if ((error = namei(&nd)) != 0) | ||||
return (error); | return (error); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
vp = nd.ni_vp; | vp = nd.ni_vp; | ||||
#ifdef MAC | |||||
error = mac_vnode_check_readlink(td->td_ucred, vp); | error = kern_readlink_vp(vp, buf, bufseg, count, td); | ||||
if (error != 0) { | |||||
vput(vp); | vput(vp); | ||||
return (error); | return (error); | ||||
} | } | ||||
/* | |||||
* Helper function to readlink from a vnode | |||||
*/ | |||||
static int | |||||
kern_readlink_vp(struct vnode *vp, char *buf, enum uio_seg bufseg, size_t count, | |||||
Not Done Inline Actionsvn_XXX namespace is for functions in vfs_vnops.c. It is somewhat strange to use it for vfs_syscalls.c kib: vn_XXX namespace is for functions in vfs_vnops.c. It is somewhat strange to use it for… | |||||
Done Inline ActionsI wasn't sure about the naming, what would you recommend? jack_gandi.net: I wasn't sure about the naming, what would you recommend? | |||||
Not Done Inline Actionskern_readlink_vp() for instance kib: kern_readlink_vp() for instance | |||||
struct thread *td) | |||||
Not Done Inline ActionsAgain indent. kib: Again indent. | |||||
{ | |||||
struct iovec aiov; | |||||
struct uio auio; | |||||
int error; | |||||
ASSERT_VOP_LOCKED(vp, "kern_readlink_vp(): vp not locked"); | |||||
#ifdef MAC | |||||
Not Done Inline ActionsI recomment to put an assert that vp is locked there. It seems that you moved vput(vp) out of the helper's code to both callers. Is this done to make the vn_readlink() interface consistent (locked vnode in/locked vnode out) ? kib: I recomment to put an assert that vp is locked there.
It seems that you moved vput(vp) out of… | |||||
Done Inline Actionsyes, I thought it would be cleaner to let the caller of vn_readlink manage the ressource jack_gandi.net: yes, I thought it would be cleaner to let the caller of `vn_readlink` manage the ressource | |||||
Done Inline ActionsIt seems that mac_vnode_check_readlink() already performs an ASSERT_VOP_LOCKED() within. jack_gandi.net: It seems that `mac_vnode_check_readlink()` already performs an `ASSERT_VOP_LOCKED()` within. | |||||
Not Done Inline Actionsmac_ stuff is conditional. Adding asserts at the start of the functions document their pre-conditions. We do not rely on asserts in the VOP_READLINK() wrapper, for similar reasons. kib: mac_ stuff is conditional. Adding asserts at the start of the functions document their pre… | |||||
error = mac_vnode_check_readlink(td->td_ucred, vp); | |||||
if (error != 0) | |||||
Not Done Inline ActionsNo need to put {} around single line. kib: No need to put {} around single line. | |||||
return (error); | |||||
#endif | #endif | ||||
if (vp->v_type != VLNK && (vp->v_vflag & VV_READLINK) == 0) | if (vp->v_type != VLNK && (vp->v_vflag & VV_READLINK) == 0) | ||||
error = EINVAL; | error = EINVAL; | ||||
else { | else { | ||||
aiov.iov_base = buf; | aiov.iov_base = buf; | ||||
aiov.iov_len = count; | aiov.iov_len = count; | ||||
auio.uio_iov = &aiov; | auio.uio_iov = &aiov; | ||||
auio.uio_iovcnt = 1; | auio.uio_iovcnt = 1; | ||||
auio.uio_offset = 0; | auio.uio_offset = 0; | ||||
auio.uio_rw = UIO_READ; | auio.uio_rw = UIO_READ; | ||||
auio.uio_segflg = bufseg; | auio.uio_segflg = bufseg; | ||||
auio.uio_td = td; | auio.uio_td = td; | ||||
auio.uio_resid = count; | auio.uio_resid = count; | ||||
error = VOP_READLINK(vp, &auio, td->td_ucred); | error = VOP_READLINK(vp, &auio, td->td_ucred); | ||||
td->td_retval[0] = count - auio.uio_resid; | td->td_retval[0] = count - auio.uio_resid; | ||||
} | } | ||||
vput(vp); | |||||
return (error); | return (error); | ||||
} | } | ||||
/* | /* | ||||
* Common implementation code for chflags() and fchflags(). | * Common implementation code for chflags() and fchflags(). | ||||
*/ | */ | ||||
static int | static int | ||||
setfflags(struct thread *td, struct vnode *vp, u_long flags) | setfflags(struct thread *td, struct vnode *vp, u_long flags) | ||||
▲ Show 20 Lines • Show All 1,555 Lines • ▼ Show 20 Lines | |||||
{ | { | ||||
struct file *fp; | struct file *fp; | ||||
int error; | int error; | ||||
error = fget_unlocked(td->td_proc->p_fd, fd, rightsp, &fp, NULL); | error = fget_unlocked(td->td_proc->p_fd, fd, rightsp, &fp, NULL); | ||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
/* | /* | ||||
Done Inline ActionsBlank line after nonexistent local variables. brooks: Blank line after nonexistent local variables. | |||||
* The file could be not of the vnode type, or it may be not | * The file could be not of the vnode type, or it may be not | ||||
* yet fully initialized, in which case the f_vnode pointer | * yet fully initialized, in which case the f_vnode pointer | ||||
Done Inline ActionsThis might be a phabricator issue, but it looks like this is indented with 2 tabs rather than a tab and four spaces. brooks: This might be a phabricator issue, but it looks like this is indented with 2 tabs rather than a… | |||||
* may be set, but f_ops is still badfileops. E.g., | * may be set, but f_ops is still badfileops. E.g., | ||||
* devfs_open() transiently create such situation to | * devfs_open() transiently create such situation to | ||||
* facilitate csw d_fdopen(). | * facilitate csw d_fdopen(). | ||||
* | * | ||||
* Dupfdopen() handling in kern_openat() installs the | * Dupfdopen() handling in kern_openat() installs the | ||||
* half-baked file into the process descriptor table, allowing | * half-baked file into the process descriptor table, allowing | ||||
* other thread to dereference it. Guard against the race by | * other thread to dereference it. Guard against the race by | ||||
* checking f_ops. | * checking f_ops. | ||||
*/ | */ | ||||
if (fp->f_vnode == NULL || fp->f_ops == &badfileops) { | if (fp->f_vnode == NULL || fp->f_ops == &badfileops) { | ||||
fdrop(fp, td); | fdrop(fp, td); | ||||
return (EINVAL); | return (EINVAL); | ||||
} | } | ||||
Done Inline ActionsDo not put initialization into local vars declaration. That said, error var is pointless, just do return (kern_getfhat());. kib: Do not put initialization into local vars declaration.
That said, error var is pointless, just… | |||||
*fpp = fp; | *fpp = fp; | ||||
return (0); | return (0); | ||||
} | } | ||||
/* | /* | ||||
* Get an (NFS) file handle. | * Get an (NFS) file handle. | ||||
*/ | */ | ||||
#ifndef _SYS_SYSPROTO_H_ | #ifndef _SYS_SYSPROTO_H_ | ||||
struct lgetfh_args { | struct lgetfh_args { | ||||
char *fname; | char *fname; | ||||
fhandle_t *fhp; | fhandle_t *fhp; | ||||
}; | }; | ||||
#endif | #endif | ||||
int | int | ||||
sys_lgetfh(struct thread *td, struct lgetfh_args *uap) | sys_lgetfh(struct thread *td, struct lgetfh_args *uap) | ||||
{ | { | ||||
return (kern_getfhat(td, AT_SYMLINK_NOFOLLOW, AT_FDCWD, uap->fname, | |||||
UIO_USERSPACE, uap->fhp)); | |||||
} | |||||
#ifndef _SYS_SYSPROTO_H_ | |||||
Done Inline ActionsThere must be a blank line after '{' if there is no locals. kib: There must be a blank line after '{' if there is no locals. | |||||
struct getfh_args { | |||||
char *fname; | |||||
fhandle_t *fhp; | |||||
Done Inline ActionsContinuation line should use +4 spaces. kib: Continuation line should use +4 spaces. | |||||
}; | |||||
#endif | |||||
int | |||||
Done Inline ActionsThis function can be static. brooks: This function can be static. | |||||
sys_getfh(struct thread *td, struct getfh_args *uap) | |||||
{ | |||||
return (kern_getfhat(td, 0, AT_FDCWD, uap->fname, UIO_USERSPACE, | |||||
uap->fhp)); | |||||
} | |||||
/* | |||||
* syscall for the rpc.lockd to use to translate an open descriptor into | |||||
* a NFS file handle. | |||||
* | |||||
* warning: do not remove the priv_check() call or this becomes one giant | |||||
* security hole. | |||||
*/ | |||||
Done Inline Actionsif (path != NULL) kib: if (path != NULL) | |||||
#ifndef _SYS_SYSPROTO_H_ | |||||
struct getfhat_args { | |||||
int fd; | |||||
char *path; | |||||
Done Inline ActionsThis indentation doesn't look like it follows style(9) brooks: This indentation doesn't look like it follows style(9) | |||||
fhandle_t *fhp; | |||||
int flags; | |||||
}; | |||||
#endif | |||||
int | |||||
sys_getfhat(struct thread *td, struct getfhat_args *uap) | |||||
{ | |||||
Done Inline ActionsNo line break brooks: No line break | |||||
if ((uap->flags & ~(AT_SYMLINK_NOFOLLOW | AT_BENEATH)) != 0) | |||||
return (EINVAL); | |||||
return (kern_getfhat(td, uap->flags, uap->fd, uap->path ? uap->path : ".", | |||||
UIO_USERSPACE, uap->fhp)); | |||||
} | |||||
static int | |||||
kern_getfhat(struct thread *td, int flags, int fd, const char *path, | |||||
enum uio_seg pathseg, fhandle_t *fhp) | |||||
{ | |||||
struct nameidata nd; | struct nameidata nd; | ||||
fhandle_t fh; | fhandle_t fh; | ||||
struct vnode *vp; | struct vnode *vp; | ||||
int error; | int error; | ||||
error = priv_check(td, PRIV_VFS_GETFH); | error = priv_check(td, PRIV_VFS_GETFH); | ||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
NDINIT(&nd, LOOKUP, NOFOLLOW | LOCKLEAF | AUDITVNODE1, UIO_USERSPACE, | NDINIT_AT(&nd, LOOKUP, ((flags & AT_SYMLINK_NOFOLLOW) != 0 ? NOFOLLOW : | ||||
uap->fname, td); | FOLLOW) | ((flags & AT_BENEATH) != 0 ? BENEATH : 0) | LOCKLEAF | | ||||
AUDITVNODE1, pathseg, path, fd, td); | |||||
error = namei(&nd); | error = namei(&nd); | ||||
if (error != 0) | if (error != 0) | ||||
Done Inline ActionsNewline before function name. brooks: Newline before function name. | |||||
return (error); | return (error); | ||||
Not Done Inline ActionsBlank line is needed after '{' since there are no local vars. kib: Blank line is needed after '{' since there are no local vars. | |||||
NDFREE(&nd, NDF_ONLY_PNBUF); | NDFREE(&nd, NDF_ONLY_PNBUF); | ||||
vp = nd.ni_vp; | vp = nd.ni_vp; | ||||
bzero(&fh, sizeof(fh)); | bzero(&fh, sizeof(fh)); | ||||
fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid; | fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid; | ||||
error = VOP_VPTOFH(vp, &fh.fh_fid); | error = VOP_VPTOFH(vp, &fh.fh_fid); | ||||
Not Done Inline ActionsPut the binary op designator before the split. flags & AT_BENEATH is not boolean, use (flags & AT_BENEATH) != 0. Same for AT_SYMLINK_NOFOLLOW. kib: Put the binary op designator before the split.
flags & AT_BENEATH is not boolean, use `(flags… | |||||
vput(vp); | vput(vp); | ||||
if (error == 0) | if (error == 0) | ||||
error = copyout(&fh, uap->fhp, sizeof (fh)); | error = copyout(&fh, fhp, sizeof (fh)); | ||||
return (error); | return (error); | ||||
} | } | ||||
#ifndef _SYS_SYSPROTO_H_ | #ifndef _SYS_SYSPROTO_H_ | ||||
struct getfh_args { | struct fhlink_args { | ||||
char *fname; | |||||
fhandle_t *fhp; | fhandle_t *fhp; | ||||
const char *to; | |||||
}; | }; | ||||
Not Done Inline ActionsLR_RETRY is allowed to return doomed vnode. You cannot access v_mount or call any VOP method on it. That said, I find having the path == NULL case allowed for userspace somewhat strange. We do not allow NULL path for open(2), and we have getfh(2). I understand that this case allows to implement sys_gethf(), I am more about checking for path == NULL in getfhat(). kib: LR_RETRY is allowed to return doomed vnode. You cannot access v_mount or call any VOP method… | |||||
Done Inline ActionsI agree the path = NULL case shouldn't be allowed for getfh, so I moved the logic into sys_getfhat. jack_gandi.net: I agree the path = NULL case shouldn't be allowed for `getfh`, so I moved the logic into… | |||||
#endif | #endif | ||||
int | int | ||||
sys_getfh(struct thread *td, struct getfh_args *uap) | sys_fhlink(struct thread *td, struct fhlink_args *uap) | ||||
{ | { | ||||
Done Inline ActionsThis can be static. brooks: This can be static. | |||||
struct nameidata nd; | |||||
return (kern_fhlinkat(td, AT_FDCWD, uap->to, UIO_USERSPACE, uap->fhp)); | |||||
} | |||||
#ifndef _SYS_SYSPROTO_H_ | |||||
struct fhlinkat_args { | |||||
Not Done Inline ActionsWrong indent. Please review all the code for style issues that were already pointed out. kib: Wrong indent. Please review all the code for style issues that were already pointed out. | |||||
fhandle_t *fhp; | |||||
int tofd; | |||||
const char *to; | |||||
}; | |||||
#endif | |||||
int | |||||
sys_fhlinkat(struct thread *td, struct fhlinkat_args *uap) | |||||
{ | |||||
return (kern_fhlinkat(td, uap->tofd, uap->to, UIO_USERSPACE, uap->fhp)); | |||||
} | |||||
static int | |||||
kern_fhlinkat(struct thread *td, int fd, const char *path, | |||||
enum uio_seg pathseg, fhandle_t *fhp) | |||||
{ | |||||
fhandle_t fh; | fhandle_t fh; | ||||
Not Done Inline ActionsAgain blank line after '{'. kib: Again blank line after '{'. | |||||
struct mount *mp; | |||||
struct vnode *vp; | struct vnode *vp; | ||||
int error; | int error; | ||||
Done Inline ActionsThis comment is pointless. Either create a helper function used by both, or remove it. kib: This comment is pointless. Either create a helper function used by both, or remove it. | |||||
Not Done Inline ActionsSo I actually mean that duplication of the code is wrong, and I really do not see why do we need two copies. kern_linkat() after the first namei() is indeed exact copy, so why not add the helper ? kib: So I actually mean that duplication of the code is wrong, and I really do not see why do we… | |||||
Done Inline ActionsI've added a helper function, the code was not _exactly_ the same so I had to change the tag/goto into a do while/EAGAIN, also has to add a locked variable. If these changes are ok by you I think it's much cleaner now. jack_gandi.net: I've added a helper function, the code was not _exactly_ the same so I had to change the… | |||||
error = priv_check(td, PRIV_VFS_GETFH); | error = priv_check(td, PRIV_VFS_GETFH); | ||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | AUDITVNODE1, UIO_USERSPACE, | error = copyin(fhp, &fh, sizeof(fh)); | ||||
uap->fname, td); | |||||
error = namei(&nd); | |||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
NDFREE(&nd, NDF_ONLY_PNBUF); | do { | ||||
vp = nd.ni_vp; | bwillwrite(); | ||||
bzero(&fh, sizeof(fh)); | if ((mp = vfs_busyfs(&fh.fh_fsid)) == NULL) | ||||
fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid; | return (ESTALE); | ||||
error = VOP_VPTOFH(vp, &fh.fh_fid); | error = VFS_FHTOVP(mp, &fh.fh_fid, LK_SHARED, &vp); | ||||
vfs_unbusy(mp); | |||||
if (error != 0) | |||||
return (error); | |||||
VOP_UNLOCK(vp, 0); | |||||
} while ((error = kern_linkat_vp(td, vp, fd, path, pathseg)) == EAGAIN); | |||||
return (error); | |||||
Not Done Inline ActionsSame question there, IMO it is worth introducing the helper and avoid copy/paste. kib: Same question there, IMO it is worth introducing the helper and avoid copy/paste. | |||||
} | |||||
#ifndef _SYS_SYSPROTO_H_ | |||||
struct fhreadlink_args { | |||||
fhandle_t *fhp; | |||||
char *buf; | |||||
size_t bufsize; | |||||
}; | |||||
#endif | |||||
int | |||||
sys_fhreadlink(struct thread *td, struct fhreadlink_args *uap) | |||||
{ | |||||
fhandle_t fh; | |||||
struct mount *mp; | |||||
struct vnode *vp; | |||||
int error; | |||||
error = priv_check(td, PRIV_VFS_GETFH); | |||||
if (error != 0) | |||||
return (error); | |||||
if (uap->bufsize > IOSIZE_MAX) | |||||
return (EINVAL); | |||||
error = copyin(uap->fhp, &fh, sizeof(fh)); | |||||
Not Done Inline ActionsRemove all blank lines from the function starting with this one, they are not needed/allowed by style. kib: Remove all blank lines from the function starting with this one, they are not needed/allowed by… | |||||
if (error != 0) | |||||
return (error); | |||||
if ((mp = vfs_busyfs(&fh.fh_fsid)) == NULL) | |||||
return (ESTALE); | |||||
Not Done Inline ActionsWhy exclusive lock ? kib: Why exclusive lock ? | |||||
Done Inline Actionsyou're right, readlinkat(2) does LOCKSHARED . behaviour should be the same with`fhreadlinkat` jack_gandi.net: you're right, `readlinkat(2)` does LOCKSHARED . behaviour should be the same with`fhreadlinkat` | |||||
error = VFS_FHTOVP(mp, &fh.fh_fid, LK_SHARED, &vp); | |||||
vfs_unbusy(mp); | |||||
if (error != 0) | |||||
return (error); | |||||
error = kern_readlink_vp(vp, uap->buf, UIO_USERSPACE, uap->bufsize, td); | |||||
Not Done Inline ActionsStray blank line, more such lines below. kib: Stray blank line, more such lines below. | |||||
vput(vp); | vput(vp); | ||||
if (error == 0) | |||||
error = copyout(&fh, uap->fhp, sizeof (fh)); | |||||
return (error); | return (error); | ||||
} | } | ||||
/* | /* | ||||
* syscall for the rpc.lockd to use to translate a NFS file handle into an | * syscall for the rpc.lockd to use to translate a NFS file handle into an | ||||
* open descriptor. | * open descriptor. | ||||
* | * | ||||
* warning: do not remove the priv_check() call or this becomes one giant | * warning: do not remove the priv_check() call or this becomes one giant | ||||
▲ Show 20 Lines • Show All 428 Lines • Show Last 20 Lines |
Wrong indent for continuation line.