Changeset View
Standalone View
sys/kern/sys_generic.c
Show First 20 Lines • Show All 101 Lines • ▼ Show 20 Lines | |||||||||||||||
#endif | #endif | ||||||||||||||
/* | /* | ||||||||||||||
* Assert that the return value of read(2) and write(2) syscalls fits | * Assert that the return value of read(2) and write(2) syscalls fits | ||||||||||||||
* into a register. If not, an architecture will need to provide the | * into a register. If not, an architecture will need to provide the | ||||||||||||||
* usermode wrappers to reconstruct the result. | * usermode wrappers to reconstruct the result. | ||||||||||||||
*/ | */ | ||||||||||||||
CTASSERT(sizeof(register_t) >= sizeof(size_t)); | CTASSERT(sizeof(register_t) >= sizeof(size_t)); | ||||||||||||||
delphij: I think it's worth commenting here that the reason we are CTASSERT'ing the size was because we… | |||||||||||||||
static MALLOC_DEFINE(M_IOCTLOPS, "ioctlops", "ioctl data buffer"); | static MALLOC_DEFINE(M_IOCTLOPS, "ioctlops", "ioctl data buffer"); | ||||||||||||||
static MALLOC_DEFINE(M_SELECT, "select", "select() buffer"); | static MALLOC_DEFINE(M_SELECT, "select", "select() buffer"); | ||||||||||||||
MALLOC_DEFINE(M_IOV, "iov", "large iov's"); | MALLOC_DEFINE(M_IOV, "iov", "large iov's"); | ||||||||||||||
static int pollout(struct thread *, struct pollfd *, struct pollfd *, | static int pollout(struct thread *, struct pollfd *, struct pollfd *, | ||||||||||||||
u_int); | u_int); | ||||||||||||||
static int pollscan(struct thread *, struct pollfd *, u_int); | static int pollscan(struct thread *, struct pollfd *, u_int); | ||||||||||||||
static int pollrescan(struct thread *); | static int pollrescan(struct thread *); | ||||||||||||||
▲ Show 20 Lines • Show All 738 Lines • ▼ Show 20 Lines | kern_posix_fallocate(struct thread *td, int fd, off_t offset, off_t len) | ||||||||||||||
error = fo_fallocate(fp, offset, len, td); | error = fo_fallocate(fp, offset, len, td); | ||||||||||||||
out: | out: | ||||||||||||||
fdrop(fp, td); | fdrop(fp, td); | ||||||||||||||
return (error); | return (error); | ||||||||||||||
} | } | ||||||||||||||
int | int | ||||||||||||||
sys_fdeallocate(struct thread *td, struct fdeallocate_args *uap) | |||||||||||||||
{ | |||||||||||||||
int error; | |||||||||||||||
error = kern_fdeallocate(td, uap->fd, uap->offset, uap->len); | |||||||||||||||
return (error); | |||||||||||||||
brooksUnsubmitted Done Inline Actions
Just returning without the temporary is more common. I noticed the 32-bit version calls kern_posix_error(). Which is intended? brooks: Just returning without the temporary is more common. I noticed the 32-bit version calls… | |||||||||||||||
khngAuthorUnsubmitted Done Inline Actionsthe kern_posix_error() is a mistake. I would remove that. khng: the kern_posix_error() is a mistake. I would remove that. | |||||||||||||||
} | |||||||||||||||
int | |||||||||||||||
kern_fdeallocate(struct thread *td, int fd, off_t offset, off_t len) | |||||||||||||||
{ | |||||||||||||||
struct file *fp; | |||||||||||||||
Done Inline ActionsI wonder if it make sense to change the fspacectl(2) interface while not too late and split in and out spacectl_range. Basically keep original spacectl_range as is, and copy out optional processed length: fspacectl(int fd, int cmd, const struct spacectl_range *sr, int flags, off_t *length); kib: I wonder if it make sense to change the fspacectl(2) interface while not too late and split in… | |||||||||||||||
Done Inline ActionsDoes it sound better to have copyout spacectl_range instead of copyout length, so that callers may save an fseek(DATA) call to guess where to restart? khng: Does it sound better to have copyout spacectl_range instead of copyout length, so that callers… | |||||||||||||||
Not Done Inline ActionsWhy do you copyout result only in case of an error? This is esp. not useful for ERESTART/EINTR etc case where you cleared the error. kib: Why do you copyout result only in case of an error? This is esp. not useful for ERESTART/EINTR… | |||||||||||||||
int error; | |||||||||||||||
Done Inline ActionsNo, I think error is more important than cerror. In other words, it should be if (error == 0) error = cerror; kib: No, I think error is more important than cerror.
In other words, it should be
```
if (error… | |||||||||||||||
AUDIT_ARG_FD(fd); | |||||||||||||||
if (offset < 0 || len <= 0) | |||||||||||||||
return (EINVAL); | |||||||||||||||
AUDIT_ARG_FD(fd); | |||||||||||||||
error = fget(td, fd, &cap_pwrite_rights, &fp); | |||||||||||||||
if (error != 0) | |||||||||||||||
return (error); | |||||||||||||||
AUDIT_ARG_FILE(td->td_proc, fp); | |||||||||||||||
if ((fp->f_ops->fo_flags & DFLAG_SEEKABLE) == 0) { | |||||||||||||||
error = ESPIPE; | |||||||||||||||
goto out; | |||||||||||||||
} | |||||||||||||||
if ((fp->f_flag & FWRITE) == 0) { | |||||||||||||||
error = EBADF; | |||||||||||||||
Not Done Inline ActionsExtra () on this and previous lines. kib: Extra () on this and previous lines. | |||||||||||||||
goto out; | |||||||||||||||
Not Done Inline Actions(flags & ~XXX) != 0 kib: (flags & ~XXX) != 0 | |||||||||||||||
} | |||||||||||||||
Done Inline ActionsDon't you need to check that offset+len do not overflow? kib: Don't you need to check that offset+len do not overflow? | |||||||||||||||
error = fo_fdeallocate(fp, offset, len, td->td_ucred, td); | |||||||||||||||
out: | |||||||||||||||
fdrop(fp, td); | |||||||||||||||
return (error); | |||||||||||||||
} | |||||||||||||||
Done Inline ActionsWhy don't you use fget_write()? I am not even sure why you check for CAP_PWRITE instead of CAP_WRITE, the later seems to be more suitable. kib: Why don't you use fget_write()? I am not even sure why you check for CAP_PWRITE instead of… | |||||||||||||||
Done Inline ActionsBut if we can in theory operate in any positions in a file, we should use CAP_PWRITE? khng: But if we can in theory operate in any positions in a file, we should use CAP_PWRITE? | |||||||||||||||
int | |||||||||||||||
sys_fzero(struct thread *td, struct fzero_args *uap) | |||||||||||||||
{ | |||||||||||||||
int error; | |||||||||||||||
error = kern_fzero(td, uap->fd, uap->offset, uap->len); | |||||||||||||||
return (error); | |||||||||||||||
} | |||||||||||||||
int | |||||||||||||||
kern_fzero(struct thread *td, int fd, off_t offset, off_t len) | |||||||||||||||
{ | |||||||||||||||
struct file *fp; | |||||||||||||||
int error; | |||||||||||||||
off_t cnt; | |||||||||||||||
Done Inline ActionsIs this intentional? If so, could you please add a comment explaining why we are ignoring ERESTART/EINTR/EWOULDBLOCK? (I think this is different from short read/write and these shouldn't be ignored?) delphij: Is this intentional? If so, could you please add a comment explaining why we are ignoring… | |||||||||||||||
Done Inline ActionsIt was intentional, but yes they shouldn't be ignored as short read/write. khng: It was intentional, but yes they shouldn't be ignored as short read/write. | |||||||||||||||
Done Inline ActionsI believe that was already discussed, and now you returned back the same errant behavior. Unix convention is that error return is only acceptable if the file content was not changed at all. If syscall did modified the file, regardless was it completed WRT requested op, or just a partial execution, userspace must know exactly what was done. kib: I believe that was already discussed, and now you returned back the same errant behavior. Unix… | |||||||||||||||
cnt = len; | |||||||||||||||
AUDIT_ARG_FD(fd); | |||||||||||||||
if (offset < 0 || len <= 0) | |||||||||||||||
return (EINVAL); | |||||||||||||||
Done Inline ActionsI still insist on copying out rmsr always, not only on error == 0. For instance, if errno == ENOSPC, you still might have modified parts of the file, and it is important for usermode to know. kib: I still insist on copying out rmsr always, not only on error == 0. For instance, if errno ==… | |||||||||||||||
AUDIT_ARG_FD(fd); | |||||||||||||||
error = fget(td, fd, &cap_pwrite_rights, &fp); | |||||||||||||||
if (error != 0) | |||||||||||||||
return (error); | |||||||||||||||
AUDIT_ARG_FILE(td->td_proc, fp); | |||||||||||||||
if ((fp->f_ops->fo_flags & DFLAG_SEEKABLE) == 0) { | |||||||||||||||
error = ESPIPE; | |||||||||||||||
goto out; | |||||||||||||||
} | |||||||||||||||
if ((fp->f_flag & FWRITE) == 0) { | |||||||||||||||
error = EBADF; | |||||||||||||||
goto out; | |||||||||||||||
} | |||||||||||||||
error = fo_fzero(fp, offset, &cnt, td->td_ucred, td); | |||||||||||||||
if (cnt != len && | |||||||||||||||
(error == ERESTART || error == EINTR || error == EWOULDBLOCK)) | |||||||||||||||
error = 0; | |||||||||||||||
len -= cnt; | |||||||||||||||
out: | |||||||||||||||
fdrop(fp, td); | |||||||||||||||
td->td_retval[0] = len; | |||||||||||||||
return (error); | |||||||||||||||
} | |||||||||||||||
int | |||||||||||||||
kern_specialfd(struct thread *td, int type, void *arg) | kern_specialfd(struct thread *td, int type, void *arg) | ||||||||||||||
{ | { | ||||||||||||||
struct file *fp; | struct file *fp; | ||||||||||||||
struct specialfd_eventfd *ae; | struct specialfd_eventfd *ae; | ||||||||||||||
int error, fd, fflags; | int error, fd, fflags; | ||||||||||||||
fflags = 0; | fflags = 0; | ||||||||||||||
error = falloc_noinstall(td, &fp); | error = falloc_noinstall(td, &fp); | ||||||||||||||
▲ Show 20 Lines • Show All 1,140 Lines • Show Last 20 Lines |
I think it's worth commenting here that the reason we are CTASSERT'ing the size was because we wanted to make sure that for both 32-bit and 64-bit variants, the struct have the same layout (and thus the compat32 call can just use the same struct); having it in the same block of asserting size_t was confusing for me in the first glance.