Re: [Minios-devel] [UNIKRAFT PATCH v3 13/23] lib/vfscore: introduce fget, fdrop, fdalloc

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Yuri,

Please find the comment inline

Thanks & Regards
Sharan

On 2/7/19 2:58 PM, Yuri Volchkov wrote:
> These functions are used in the imported code, which is not enabled
> yet. This required to expand functionality of vfscore_install_fd and
> friends with locking and reference counting.
>
> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
> ---
>   lib/vfscore/fd.c                   | 67 ++++++++++++++++++++++++++++--
>   lib/vfscore/file.c                 | 22 +++++++++-
>   lib/vfscore/include/vfscore/file.h |  9 +++-
>   3 files changed, 92 insertions(+), 6 deletions(-)
>
> diff --git a/lib/vfscore/fd.c b/lib/vfscore/fd.c
> index 6c220428..9d2afe5a 100644
> --- a/lib/vfscore/fd.c
> +++ b/lib/vfscore/fd.c
> @@ -74,20 +74,41 @@ exit:
>   
>   void vfscore_put_fd(int fd)
>   {
> +       unsigned long flags;
> +
>         UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
>         /* Currently it is not allowed to free std(in|out|err) */
>         UK_ASSERT(fd > 2);
>   
> -	__uk_clear_bit(fd, &fdtable.bitmap);
> +       flags = ukplat_lcpu_save_irqf();
> +       __uk_clear_bit(fd, &fdtable.bitmap);\
> +       fdtable.files[fd] = NULL;
> +       ukplat_lcpu_restore_irqf(flags);
Shouldn't we have a fdrop on the vfscore_file descriptor?

>   }
>   
> -void vfscore_install_fd(int fd, struct vfscore_file *file)
> +int vfscore_install_fd(int fd, struct vfscore_file *file)
>   {
> -       UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       UK_ASSERT(file);
> +       unsigned long flags;
> +       struct vfscore_file *orig;
> +
> +       if ((fd >= (int) FDTABLE_MAX_FILES) || (!file))
> +               return -EBADF;
> +
> +       fhold(file);
>   
>   	file->fd = fd;
> +
> +       flags = ukplat_lcpu_save_irqf();
Shouldn't we check if there active reference to the orig before 
installing the fd?
> +       orig = fdtable.files[fd];
>         fdtable.files[fd] = file;
> +       ukplat_lcpu_restore_irqf(flags);
> +
> +       fdrop(file);
> +
> +       if (orig)
This reference has to be closed as there can be no further reference to it.
> +               fdrop(orig);
> +
> +       return 0;
>   }
>   
>   struct vfscore_file *vfscore_get_file(int fd)
> @@ -101,12 +122,50 @@ struct vfscore_file *vfscore_get_file(int fd)
>         if (!(fdtable.bitmap & ((uint64_t) 1 << fd)))
>                 goto exit;
>         ret = fdtable.files[fd];
> +       fhold(ret);
>   
>   exit:
>         ukplat_lcpu_restore_irqf(flags);
>         return ret;
>   }
>   
> +int fget(int fd, struct vfscore_file **out_fp)
> +{
> +       int ret = 0;
> +       struct vfscore_file *fp = vfscore_get_file(fd);
> +
> +       if (!fp)
We are using both -ve and +ve error within this file. Maybe wise to
stick to a single scheme. Is it because of interaction with the imported 
OSv code?
> +               ret = EBADF;
> +       else
> +               *out_fp = fp;
> +
> +       return ret;
> +}
> +
> +int fdalloc(struct vfscore_file *fp, int *newfd)
> +{
> +       int fd, ret = 0;
> +
> +       fhold(fp);
> +
> +       fd = vfscore_alloc_fd();
> +       if (fd < 0) {
Shouldn't we have an fdrop here?

> +               ret = fd;
> +               goto exit;
> +       }
> +
> +       ret = vfscore_install_fd(fd, fp);
> +       if (ret)
> +               fdrop(fp);
> +       else
> +               *newfd = fd;
> +
> +exit:
> +       return ret;
> +}
> +
> +
> +/* TODO: move this constructor to main.c */
>   __constructor static void fdtable_init(void)
>   {
>         memset(&fdtable, 0, sizeof(fdtable));
> diff --git a/lib/vfscore/file.c b/lib/vfscore/file.c
> index e7adf1a3..0f6dfa93 100644
> --- a/lib/vfscore/file.c
> +++ b/lib/vfscore/file.c
> @@ -38,7 +38,7 @@
>   #include <uk/print.h>
>   #include <vfscore/file.h>
>   #include <uk/assert.h>
> -
> +#include <uk/arch/atomic.h>
>   
>   int close(int fd)
>   {
> @@ -95,3 +95,23 @@ ssize_t read(int fd, void *buf, size_t count)
>   
>   	return file->fops->read(file, buf, count);
>   }
> +
> +/* TODO: remove stub */
> +#define vfs_close(fp) (0)
> +
For reference counting we could use the <include/uk/refcount.h>. As 

                                                              mentioned 
in the previous patch it is not necessary for this patch 

series.
> +int fdrop(struct vfscore_file *fp)
> +{
> +       int ret = 0;
> +       int prev = ukarch_dec(&fp->f_count);
> +
> +       if (prev == 0)
> +               UK_CRASH("Unbalanced fhold/fdrop");
> +
> +       if (prev == 1)
> +               ret = vfs_close(fp);
> +       return ret;
> +}
> +void fhold(struct vfscore_file *fp)
> +{
> +       ukarch_inc(&fp->f_count);
> +}
> diff --git a/lib/vfscore/include/vfscore/file.h 
> b/lib/vfscore/include/vfscore/file.h
> index 3d8d667f..84eb35d7 100644
> --- a/lib/vfscore/include/vfscore/file.h
> +++ b/lib/vfscore/include/vfscore/file.h
> @@ -53,14 +53,21 @@ struct vfscore_fops {
>   
>   struct vfscore_file {
>         int fd;
> +       int             f_count;        /* reference count */
>         const struct vfscore_fops *fops;
>   };
>   
>   int vfscore_alloc_fd(void);
>   void vfscore_put_fd(int fd);
> -void vfscore_install_fd(int fd, struct vfscore_file *file);
> +int vfscore_install_fd(int fd, struct vfscore_file *file);
>   struct vfscore_file *vfscore_get_file(int fd);
>   
> +/*
> + * File descriptors reference count
> + */
> +void fhold(struct vfscore_file* fp);
> +int fdrop(struct vfscore_file* fp);
> +
>   #define FOF_OFFSET  0x0800    /* Use the offset in uio argument */
>   
>   #ifdef __cplusplus
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel