|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH] lib/vfscore: Add anonymous pipe
Hi Bogdan
Thanks for this patch, we certainly need the pipe primitive for
Unikraft. However, this patch needs a bit of rework. First of all, it
should also compile using nolibc, but it fails now because 'sys/ioctl.h'
is missing from nolibc.
For the rest of the comments, please see inline.
On 6/29/19 1:03 PM, Bogdan Lascu wrote:
> Adds pipe function that creates an anonymous pipe and creates functions
> that write and read to a pipe file descriptor.
>
> Signed-off-by: Bogdan Lascu <lascu.bogdan96@xxxxxxxxx>
> ---
> lib/vfscore/Makefile.uk | 1 +
> lib/vfscore/exportsyms.uk | 1 +
> lib/vfscore/include/vfscore/pipe.h | 67 ++++++
> lib/vfscore/pipe.c | 409
> +++++++++++++++++++++++++++++++++++++
> 4 files changed, 478 insertions(+)
> create mode 100644 lib/vfscore/include/vfscore/pipe.h
> create mode 100644 lib/vfscore/pipe.c
>
> diff --git a/lib/vfscore/Makefile.uk b/lib/vfscore/Makefile.uk
> index 0166e612..4e519367 100644
> --- a/lib/vfscore/Makefile.uk
> +++ b/lib/vfscore/Makefile.uk
> @@ -14,6 +14,7 @@ LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/task.c
> LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/lookup.c
> LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/fops.c
> LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/subr_uio.c
> +LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/pipe.c
>
> EXTRA_LD_SCRIPT-$(CONFIG_LIBVFSCORE) += $(LIBVFSCORE_BASE)/extra.ld
>
> diff --git a/lib/vfscore/exportsyms.uk b/lib/vfscore/exportsyms.uk
> index da7fbaea..16a0f86a 100644
> --- a/lib/vfscore/exportsyms.uk
> +++ b/lib/vfscore/exportsyms.uk
> @@ -57,6 +57,7 @@ writev
> truncate
> mknod
> preadv
> +pipe
> ioctl
> fdatasync
> fdopendir
> diff --git a/lib/vfscore/include/vfscore/pipe.h
> b/lib/vfscore/include/vfscore/pipe.h
> new file mode 100644
> index 00000000..0c25f4ed
> --- /dev/null
> +++ b/lib/vfscore/include/vfscore/pipe.h
> @@ -0,0 +1,67 @@
> +/* pipe.h - pipe header
> + *
> + * Authors: Bogdan-George Lascu <lascu.bogdan96@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, University Politehnica of Bucharest. All rights
> reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +#include <vfscore/file.h>
> +#include <uk/wait.h>
> +#include <uk/mutex.h>
> +
> +#define PIPE_MAX_SIZE (1 << 16)
> +
> +
> +struct pipe_buf {
> + char *data;
> + unsigned int max_size;
> + size_t len; /* Total len of buffer. */
> + unsigned int r_off;
> + unsigned int w_off;
> +};
> +
> +struct pipe_info {
> + struct pipe_buf *buf;
> + int w_count;
> + int r_count;
> + int flags;
> + struct uk_mutex pipe_lock;
> + struct uk_waitq wq;
> +};
It would help to add some comments for each structure and for each of
the structure fields.
> +
> +struct pipe_buf *alloc_pipe_buf(int size);
> +struct pipe_info *alloc_pipe_info(int size, int flags);
> +
> +void free_pipe_buf(struct pipe_buf *pipe_buf);
> +void free_pipe_info(struct pipe_info *pipe_info);
The same applies for these functions.
> +
> +
> +#define PIPE_LOCK(__pipe) uk_mutex_lock(&__pipe->pipe_lock)
> +#define PIPE_UNLOCK(__pipe) uk_mutex_unlock(&__pipe->pipe_lock)
I fail to understand why we use this header with all these declarations
considering that the only public function of the pipe implementation is
the POSIX pipe().
> +
> diff --git a/lib/vfscore/pipe.c b/lib/vfscore/pipe.c
> new file mode 100644
> index 00000000..15137627
> --- /dev/null
> +++ b/lib/vfscore/pipe.c
> @@ -0,0 +1,409 @@
> +/* pipe.c - pipe implementation
> + *
> + * Authors: Bogdan-George Lascu <lascu.bogdan96@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, University Politehnica of Bucharest. All rights
> reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <vfscore/pipe.h>
> +#include <vfscore/file.h>
> +#include <vfscore/vnode.h>
> +#include <vfscore/fs.h>
> +#include <uk/wait.h>
> +#include <sys/ioctl.h>
> +
> +
> +size_t get_pipe_buf_free_space(struct pipe_buf *pipe_buf)
> +{
> + return pipe_buf->max_size - pipe_buf->len;
> +}
> +
> +int pipe_can_read(struct pipe_buf *pipe_buf)
> +{
> + return pipe_buf->len > 0;
> +}
> +
> +/* Write blocks until there is enough space to write the entire user buffer.
> */
> +int pipe_can_write(struct pipe_buf *pipe_buf, size_t user_buf_len)
> +{
> + return get_pipe_buf_free_space(pipe_buf) >= user_buf_len;
> +}
> +
> +
> +static int pipe_write(struct vnode *vnode,
> + struct uio *buf, int ioflag __unused)
> +{
> + struct pipe_info *pipe = vnode->v_data;
> + struct pipe_buf *pipe_buf = pipe->buf;
> + struct iovec iovec;
> + int to_write = 0;
> + int i;
> +
> + if (!pipe->r_count) {
> + // TODO before returning the error, send a SIGPIPE signal.
> + return -EPIPE;
> + }
> +
> + for (i = 0; i < buf->uio_iovcnt; i++)
> + to_write += buf->uio_iov[i].iov_len;
> +
> + uk_waitq_wait_event(&pipe->wq, pipe_can_write(pipe_buf, to_write));
> + PIPE_LOCK(pipe);
> + for (i = 0; i < buf->uio_iovcnt; i++) {
> + iovec = buf->uio_iov[i];
> +
> + /* Copy the user buffer in pipe buffer. */
> + if (pipe_buf->w_off + iovec.iov_len <= pipe_buf->max_size) {
> + memcpy(pipe_buf->data + pipe_buf->w_off,
> + iovec.iov_base,
> + iovec.iov_len);
> + } else {
> + int first_copy_bytes;
> + int second_copy_bytes;
> +
> + /* Copy the from current offset to the end. */
> + first_copy_bytes = pipe_buf->max_size - pipe_buf->w_off;
> + memcpy(pipe_buf->data + pipe_buf->w_off,
> + iovec.iov_base,
> + first_copy_bytes);
> +
> + /* Copy from the begging the remaining bytes. */
> + second_copy_bytes = pipe_buf->w_off
> + + iovec.iov_len
> + - pipe_buf->max_size;
> +
> + memcpy(pipe_buf->data,
> + iovec.iov_base + first_copy_bytes,
> + second_copy_bytes);
> +
> + }
> +
> + /* Update buffer write offset. */
> + pipe_buf->w_off = (pipe_buf->w_off + iovec.iov_len)
> + % pipe_buf->max_size;
> +
> + /* Update pipe buf len. */
> + pipe_buf->len += iovec.iov_len;
> +
> + /* Update bytes written. */
> + buf->uio_resid -= iovec.iov_len;
> + }
> + PIPE_UNLOCK(pipe);
> + uk_waitq_wake_up(&pipe->wq);
> +
> + return 0;
> +}
> +
> +static int pipe_read(struct vnode *vnode,
> + struct vfscore_file *vfscore_file __unused,
> + struct uio *buf, int ioflag __unused)
> +{
> + struct pipe_info *pipe = vnode->v_data;
> + struct pipe_buf *pipe_buf = pipe->buf;
> + struct iovec iovec;
> + int to_read;
> + int i;
> +
> + if ((vfscore_file->f_flags & O_NONBLOCK) && pipe_buf->len == 0)
> + return -EAGAIN;
> +
> + uk_waitq_wait_event(&pipe->wq, pipe_can_read(pipe_buf));
> + PIPE_LOCK(pipe);
> + for (i = 0; i < buf->uio_iovcnt; i++) {
> + iovec = buf->uio_iov[i];
> + to_read = MIN(iovec.iov_len, pipe_buf->len);
> + if (to_read == 0)
> + break;
> +
> + /* Copy from pipe buffer to user buffer. */
> + if (pipe_buf->r_off + to_read <= pipe_buf->max_size) {
> + memcpy(iovec.iov_base,
> + pipe_buf->data + pipe_buf->r_off,
> + to_read);
> + } else {
> + int first_copy_bytes;
> + int second_copy_bytes;
> +
> + /* Copy the from current offset to the end. */
> + first_copy_bytes = pipe_buf->max_size - pipe_buf->r_off;
> + memcpy(iovec.iov_base,
> + pipe_buf->data + pipe_buf->r_off,
> + first_copy_bytes);
> +
> + /* Copy from the begging the remaining bytes. */
> + second_copy_bytes = pipe_buf->r_off
> + + to_read
> + - pipe_buf->max_size;
> +
> + memcpy(iovec.iov_base + first_copy_bytes,
> + pipe_buf->data,
> + second_copy_bytes);
> +
> + }
> +
> + /* Update read buffer offset. */
> + pipe_buf->r_off = (pipe_buf->r_off + to_read) %
> pipe_buf->max_size;
You have a checkpatch warning here. I think you can put the assigned
value on the next line.
> +
> + /* Update pipe buffer len. */
> + pipe_buf->len -= to_read;
> +
> + /* Update bytes read which will be returned. */
> + buf->uio_resid -= to_read;
> + }
> + PIPE_UNLOCK(pipe);
> + uk_waitq_wake_up(&pipe->wq);
> +
> + return 0;
> +}
> +
> +static int pipe_close(struct vnode *vnode,
> + struct vfscore_file *file)
> +{
> + struct pipe_info *pipe = vnode->v_data;
> +
> + PIPE_LOCK(pipe);
> +
> + if (file->f_flags & UK_FREAD)
> + pipe->r_count--;
> +
> + if (file->f_flags & UK_FWRITE)
> + pipe->w_count--;
> +
> + if (!pipe->r_count && !pipe->w_count)
> + free_pipe_info(pipe);
> +
> + PIPE_UNLOCK(pipe);
> +
> + return 0;
> +}
> +
> +static int pipe_seek(struct vnode *vnode __unused,
> + struct vfscore_file *file __unused,
> + off_t off1 __unused, off_t off2 __unused)
> +{
> + return -EPIPE;
> +}
> +
> +static int pipe_ioctl(struct vnode *vnode, struct vfscore_file *file
> __unused,
> + unsigned long com, void *data __unused)
> +{
> + struct pipe_info *pipe = vnode->v_data;
> + struct pipe_buf *pipe_buf __unused = pipe->buf;
> +
> + switch (com) {
> +#ifdef FIONREAD
> + case FIONREAD:
> + PIPE_LOCK(pipe);
> + *(int *)data = pipe_buf->len;
> + PIPE_UNLOCK(pipe);
> + return 0;
> +#endif
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static struct vnops pipe_fops = {
> + .vop_read = pipe_read,
> + .vop_write = pipe_write,
> + .vop_close = pipe_close,
> + .vop_seek = pipe_seek,
> + .vop_ioctl = pipe_ioctl,
> +};
> +
> +struct pipe_buf *alloc_pipe_buf(int max_size)
> +{
> + struct pipe_buf *pipe_buf;
> +
> + pipe_buf = uk_calloc(uk_alloc_get_default(), 1, sizeof(*pipe_buf));
> + if (!pipe_buf)
> + return NULL;
> + pipe_buf->data = uk_calloc(uk_alloc_get_default(), 1,
> + max_size * sizeof(char));
> + if (!pipe_buf->data) {
> + uk_free(uk_alloc_get_default(), pipe_buf);
> + return NULL;
> + }
> +
> + pipe_buf->r_off = 0;
> + pipe_buf->w_off = 0;
> + pipe_buf->max_size = max_size;
> + pipe_buf->len = 0;
> +
> + return pipe_buf;
> +}
> +
> +struct pipe_info *alloc_pipe_info(int size, int flags)
> +{
> + struct pipe_info *pipe_info;
> +
> + pipe_info = uk_calloc(uk_alloc_get_default(), 1, sizeof(*pipe_info));
> + if (!pipe_info)
> + return NULL;
> +
> + pipe_info->r_count = 1;
> + pipe_info->w_count = 1;
> + pipe_info->flags = flags;
> +
> + pipe_info->buf = alloc_pipe_buf(size);
> + if (!pipe_info->buf) {
> + uk_free(uk_alloc_get_default(), pipe_info);
> + return NULL;
> + }
> +
> + uk_mutex_init(&pipe_info->pipe_lock);
> + uk_waitq_init(&pipe_info->wq);
> +
> + return pipe_info;
> +}
> +
> +void free_pipe_buf(struct pipe_buf *pipe_buf)
> +{
> + uk_free(uk_alloc_get_default(), pipe_buf->data);
> + uk_free(uk_alloc_get_default(), pipe_buf);
> +}
> +void free_pipe_info(struct pipe_info *pipe_info)
> +{
> + free_pipe_buf(pipe_info->buf);
> + uk_free(uk_alloc_get_default(), pipe_info);
> +}
> +
> +
> +int pipe(int pipefd[2])
> +{
> + int ret = 0;
> + int w_fd, r_fd;
> + struct dentry *w_dentry, *r_dentry;
> + struct vnode *r_vnode, *w_vnode;
> + struct vfscore_file *r_file, *w_file;
> + struct pipe_info *pipe_info;
> +
> + /* Reserve file descriptor number. */
> + r_fd = vfscore_alloc_fd();
> + w_fd = vfscore_alloc_fd();
> + if (r_fd < 0 || w_fd < 0) {
> + ret = -ENFILE;
> + goto ERR_EXIT;
> + }
> +
> + /* Allocate file, dentry, and vnode. */
> + r_file = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_file));
> + w_file = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_file));
> + if (!r_file || !w_file) {
> + ret = -ENOMEM;
> + goto ERR_MALLOC_FILE;
> + }
> +
> + r_dentry = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_dentry));
> + w_dentry = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_dentry));
> + if (!r_dentry || !w_dentry) {
> + ret = -ENOMEM;
> + goto ERR_MALLOC_DENTRY;
> + }
> +
> + r_vnode = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_vnode));
> + w_vnode = uk_calloc(uk_alloc_get_default(), 1, sizeof(*r_vnode));
> + if (!r_vnode || !w_vnode) {
> + ret = -ENOMEM;
> + goto ERR_MALLOC_VNODE;
> + }
> +
> + /* Allocate pipe internal structure. */
> + pipe_info = alloc_pipe_info(PIPE_MAX_SIZE, 0);
> + if (!pipe_info) {
> + ret = -ENOMEM;
> + goto ERR_ALLOC_PIPE_INFO;
> + }
> +
> + /* Fill out necessary fields. */
> + r_file->fd = r_fd;
> + w_file->fd = r_fd;
> +
> + r_file->f_count = 1;
> + w_file->f_count = 1;
> +
> + r_file->f_flags = UK_FREAD;
> + w_file->f_flags = UK_FWRITE;
> +
> + r_file->f_dentry = r_dentry;
> + w_file->f_dentry = w_dentry;
> +
> + r_dentry->d_vnode = r_vnode;
> + w_dentry->d_vnode = w_vnode;
> +
> +
> + r_vnode->v_op = &pipe_fops;
> + w_vnode->v_op = &pipe_fops;
> +
> + uk_mutex_init(&r_vnode->v_lock);
> + uk_mutex_init(&w_vnode->v_lock);
> +
> + r_vnode->v_refcnt = 1;
> + w_vnode->v_refcnt = 1;
> +
> + r_vnode->v_data = pipe_info;
> + w_vnode->v_data = pipe_info;
> +
> + /* Assign the file descriptors to the corresponding vfs_file. */
> + ret = vfscore_install_fd(r_fd, r_file);
> + if (ret)
> + goto ERR_VFS_INSTALL;
> +
> + ret = vfscore_install_fd(w_fd, w_file);
> + if (ret)
> + goto ERR_VFS_INSTALL;
> +
> + /* Fill pipefd fields. */
> + pipefd[0] = r_fd;
> + pipefd[1] = w_fd;
There is so much duplicated code in this function, can't we make a
static function at least for the common bits, the bits that have exactly
the same initialization values?
> +
> + return ret;
> +
> +ERR_VFS_INSTALL:
> + free_pipe_info(pipe_info);
> +ERR_ALLOC_PIPE_INFO:
> + uk_free(uk_alloc_get_default(), r_vnode);
> + uk_free(uk_alloc_get_default(), w_vnode);
> +ERR_MALLOC_VNODE:
> + uk_free(uk_alloc_get_default(), r_dentry);
> + uk_free(uk_alloc_get_default(), w_dentry);
> +ERR_MALLOC_DENTRY:
> + uk_free(uk_alloc_get_default(), r_file);
> + uk_free(uk_alloc_get_default(), w_file);
> +ERR_MALLOC_FILE:
> + vfscore_put_fd(r_fd);
> + vfscore_put_fd(w_fd);
> +ERR_EXIT:
> + UK_ASSERT(ret < 0);
> + return ret;
> +}
> +
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |