Re: [Minios-devel] [UNIKRAFT PATCH 2/6] lib/vfscore: Fix dup2()

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Looks good to me. Thanks!
Can you update the subject to something more precise, like
"lib/vfscore: dup2(): Close newfd when already opened"

Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>

On 06.12.19 14:41, Costin Lupu wrote:
> If newfd is open, then POSIX says we have to close it - that's what this
> patch aims to achieve. Given that dup2/dup3 explicitly wants to reserver a
> file descriptor, we also introduce the vfscore_reserve_fd() function. We
> also add a tiny fix in vfscore_alloc_fd() because it should also be able to
> allocate file descriptor 0 - that's what stdio initialization needs among
> other things.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/vfscore/fd.c                   | 22 +++++++++++++++++++---
>   lib/vfscore/include/vfscore/file.h |  1 +
>   lib/vfscore/main.c                 | 14 +++++++++++++-
>   lib/vfscore/stdio.c                |  4 ++++
>   4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/lib/vfscore/fd.c b/lib/vfscore/fd.c
> index 7efed741..6db366a9 100644
> --- a/lib/vfscore/fd.c
> +++ b/lib/vfscore/fd.c
> @@ -61,7 +61,7 @@ int vfscore_alloc_fd(void)
>         flags = ukplat_lcpu_save_irqf();
>         ret = uk_find_next_zero_bit(fdtable.bitmap, FDTABLE_MAX_FILES, 0);
>   
> -	if (!ret) {
> +       if (ret == FDTABLE_MAX_FILES) {
>                 ret = -ENFILE;
>                 goto exit;
>         }
> @@ -73,6 +73,24 @@ exit:
>         return ret;
>   }
>   
> +int vfscore_reserve_fd(int fd)
> +{
> +       unsigned long flags;
> +       int ret = 0;
> +
> +       flags = ukplat_lcpu_save_irqf();
> +       if (uk_test_bit(fd, fdtable.bitmap)) {
> +               ret = -EBUSY;
> +               goto exit;
> +       }
> +
> +       uk_bitmap_set(fdtable.bitmap, fd, 1);
> +
> +exit:
> +       ukplat_lcpu_restore_irqf(flags);
> +       return ret;
> +}
> +
>   int vfscore_put_fd(int fd)
>   {
>         struct vfscore_file *fp;
> @@ -188,8 +206,6 @@ static void fdtable_init(void)
>   {
>         memset(&fdtable, 0, sizeof(fdtable));
>   
> -	/* reserve stdin, stdout and stderr */
> -       uk_bitmap_set(fdtable.bitmap, 0, 3);
>         init_stdio();
>   }
>   
> diff --git a/lib/vfscore/include/vfscore/file.h b/lib/vfscore/include/vfscore/file.h
> index c698201d..be239744 100644
> --- a/lib/vfscore/include/vfscore/file.h
> +++ b/lib/vfscore/include/vfscore/file.h
> @@ -66,6 +66,7 @@ struct vfscore_file {
>   #define FD_UNLOCK(fp)     uk_mutex_unlock(&(fp->f_lock))
>   
>   int vfscore_alloc_fd(void);
> +int vfscore_reserve_fd(int fd);
>   int vfscore_put_fd(int fd);
>   int vfscore_install_fd(int fd, struct vfscore_file *file);
>   struct vfscore_file *vfscore_get_file(int fd);
> diff --git a/lib/vfscore/main.c b/lib/vfscore/main.c
> index d6145a2b..4beda603 100644
> --- a/lib/vfscore/main.c
> +++ b/lib/vfscore/main.c
> @@ -1379,7 +1379,7 @@ UK_TRACEPOINT(trace_vfs_dup3_err, "%d", int);
>    */
>   int dup3(int oldfd, int newfd, int flags)
>   {
> -       struct vfscore_file *fp;
> +       struct vfscore_file *fp, *fp_new;
>         int error;
>   
>   	trace_vfs_dup3(oldfd, newfd, flags);
> @@ -1401,6 +1401,18 @@ int dup3(int oldfd, int newfd, int flags)
>         if (error)
>                 goto out_errno;
>   
> +	error = fget(newfd, &fp_new);
> +       if (error == 0) {
> +               /* if newfd is open, then close it */
> +               error = close(newfd);
> +               if (error)
> +                       goto out_errno;
> +       }
> +
> +       error = vfscore_reserve_fd(newfd);
> +       if (error)
> +               goto out_errno;
> +
>         error = vfscore_install_fd(newfd, fp);
>         if (error) {
>                 fdrop(fp);
> diff --git a/lib/vfscore/stdio.c b/lib/vfscore/stdio.c
> index 578b20bb..62a7a607 100644
> --- a/lib/vfscore/stdio.c
> +++ b/lib/vfscore/stdio.c
> @@ -211,6 +211,10 @@ static struct vfscore_file  stdio_file = {
>   
>   void init_stdio(void)
>   {
> +       int fd;
> +
> +       fd = vfscore_alloc_fd();
> +       UK_ASSERT(fd == 0);
>         vfscore_install_fd(0, &stdio_file);
>         if (dup2(0, 1) != 1)
>                 uk_pr_err("failed to dup to stdin\n");
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel