[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH] lib/vfscore: Add anonymous pipe
Hey Bogdan,
can you also make sure that the pipe functionality is going to a
separate (internal) library and not part of vfscore - for instance call
it ukpipe? This way we can keep feature selection as flexible as possible.
Thanks,
Simon
On 01.07.19 12:29, Costin Lupu wrote:
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
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
