On Wed, Sep 18, 2013 at 05:56:04PM +0000, Roman Divacky wrote:
> Author: rdivacky
> Date: Wed Sep 18 17:56:04 2013
> New Revision: 255672
> URL: http://svnweb.freebsd.org/changeset/base/255672
>
> Log:
> Implement epoll support in Linuxulator. This is a tiny wrapper around kqueue
> to implement epoll subset of functionality. The kqueue user data are 32bit
> on i386 which is not enough for epoll user data so this patch overrides
> kqueue fileops to maintain enough space in struct file.
>
> Initial patch developed by me in 2007 and then extended and finished
> by Yuri Victorovich.
>
First of all thank you both for doing this work.
I have some important though (I didn't spend too much on this, so maybe
I missed some stuff or what I write here is incorrect).
In general some lines are longer than 80 characters and simple stile
violations are present ("if (!error)").
> +/* Create a new epoll file descriptor. */
> +
> +static int
> +linux_epoll_create_common(struct thread *td)
> +{
> + struct file *fp;
> + int error;
> +
> + error = kern_kqueue_locked(td, &fp);
> +#if EPOLL_WIDE_USER_DATA
> + if (error == 0) {
> + epoll_init_user_data(td, fp);
> + fdrop(fp, td);
> + }
> +#endif
> + return (error);
> +}
This leaks fd reference if EPOLL_WIDE_USER_DATA is not defined.
> +int
> +linux_epoll_create1(struct thread *td, struct linux_epoll_create1_args *args)
> +{
> + int error;
> +
> + error = linux_epoll_create_common(td);
> +
> + if (!error) {
> + if (args->flags & LINUX_EPOLL_CLOEXEC)
> +
> td->td_proc->p_fd->fd_ofiles[td->td_retval[0]].fde_flags |= UF_EXCLOSE;
This is very racy for no good reason. This should be passed down to
kqueue and be set on fd creation.
> + if (args->flags & LINUX_EPOLL_NONBLOCK)
> + linux_msg(td, "epoll_create1 doesn't yet support
> EPOLL_NONBLOCK flag\n");
> + }
> +
> + return (error);
> +}
> +
> +
> +static void
> +epoll_init_user_data(struct thread *td, struct file *epfp)
> +{
> + struct epoll_user_data *udv;
> +
> + /* override file ops to have our close operation */
> + atomic_store_rel_ptr((volatile uintptr_t *)&epfp->f_ops,
> (uintptr_t)&epollops);
> +
> + /* allocate epoll_user_data initially for up to 16 file descriptor
> values */
> + udv = malloc(EPOLL_USER_DATA_SIZE(EPOLL_USER_DATA_MARGIN),
> M_LINUX_EPOLL, M_WAITOK);
> + udv->sz = EPOLL_USER_DATA_MARGIN;
> + EPOLL_USER_DATA_SET(epfp, udv);
> +}
Is not this racy? There is a window when fd is installed with epoll ops,
yet no userdata is allocated.
> +/*ARGSUSED*/
> +static int
> +epoll_close(struct file *epfp, struct thread *td)
> +{
> + /* free user data vector */
> + free(EPOLL_USER_DATA_GET(epfp), M_LINUX_EPOLL);
> + /* over to kqueue parent */
> + return (kqueue_close(epfp, td));
> +}
> +#endif
Unnecessary comments.
> +
> +static struct file*
> +epoll_fget(struct thread *td, int epfd)
> +{
> + struct file *fp;
> + cap_rights_t rights;
> +
> + if (fget(td, epfd, cap_rights_init(&rights, CAP_POLL_EVENT), &fp) != 0)
> + panic("epoll: no file object found for kqueue descriptor");
> +
> + return (fp);
> +}
> +
Callers pass arbitrary fd here (provided by the user), yet this panics
if fd is bad.
> int
> +kern_kqueue(struct thread *td)
> +{
> + struct file *fp;
> + int error;
> +
> + error = kern_kqueue_locked(td, &fp);
> +
Why is this _locked? Typically such naming is used when some locks are
held around the call.
> + fdrop(fp, td);
If there was an error, fdrop is called even though there is nothing to
fdrop.
> + return (error);
> +}
--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
