Re: [Minios-devel] [UNIKRAFT PATCH v3 2/2] lib/vfscore: implement fops for std(out|err)

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hey,

I have some minor comments inline. It is more about preference about 
deduplication. But in general I am fine with this patch.
Thanks,

Simon

On 14.06.2018 16:53, Yuri Volchkov wrote:
> Normally printf-alike functions are writing to the file 1 or 2. This
> patch provides necessary file operations for it to work
>
> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
> ---
>   lib/vfscore/Makefile.uk       |  1 +
>   lib/vfscore/fd.c              |  5 ++-
>   lib/vfscore/{fd.c => stdio.c} | 78 +++++++++++++----------------------
>   3 files changed, 33 insertions(+), 51 deletions(-)
>   copy lib/vfscore/{fd.c => stdio.c} (61%)
Wow, this is confusing ;-)

> diff --git a/lib/vfscore/Makefile.uk b/lib/vfscore/Makefile.uk
> index fa56c8e..695b357 100644
> --- a/lib/vfscore/Makefile.uk
> +++ b/lib/vfscore/Makefile.uk
> @@ -4,3 +4,4 @@ CINCLUDES-y += -I$(LIBVFSCORE_BASE)/include
>   
>   LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/fd.c
>   LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/file.c
> +LIBVFSCORE_SRCS-y += $(LIBVFSCORE_BASE)/stdio.c
> \ No newline at end of file
> diff --git a/lib/vfscore/fd.c b/lib/vfscore/fd.c
> index 85761ea..10e570c 100644
> --- a/lib/vfscore/fd.c
> +++ b/lib/vfscore/fd.c
> @@ -41,6 +41,8 @@
>   
>   #define FDTABLE_MAX_FILES (sizeof(uint64_t) * 8)
>   
> +void init_stdio(void);
> +
>   struct fdtable {
>         uint64_t bitmap;
>         uint32_t fd_start;
> @@ -72,7 +74,6 @@ void vfscore_put_fd(int fd)
>   void vfscore_install_fd(int fd, struct vfscore_file *file)
>   {
>         UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       UK_ASSERT(fd > 2);
>         UK_ASSERT(file);
>   
>   	file->fd = fd;
> @@ -82,7 +83,6 @@ void vfscore_install_fd(int fd, struct vfscore_file *file)
>   struct vfscore_file *vfscore_get_file(int fd)
>   {
>         UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       UK_ASSERT(fd > 2);
>   
>   	if (!(fdtable.bitmap & ((uint64_t) 1 << fd)))
>                 return NULL;
> @@ -96,4 +96,5 @@ __constructor static void fdtable_init(void)
>   
>   	/* reserve stdin, stdout and stderr */
>         fdtable.bitmap = 7;
> +       init_stdio();
>   }
> diff --git a/lib/vfscore/fd.c b/lib/vfscore/stdio.c
> similarity index 61%
> copy from lib/vfscore/fd.c
> copy to lib/vfscore/stdio.c
> index 85761ea..e4e5964 100644
> --- a/lib/vfscore/fd.c
> +++ b/lib/vfscore/stdio.c
> @@ -33,67 +33,47 @@
>    * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>    */
>   
> -#include <string.h>
> -#include <uk/essentials.h>
> -#include <uk/arch/atomic.h>
> -#include <uk/assert.h>
>   #include <vfscore/file.h>
> +#include <uk/plat/console.h>
>   
> -#define FDTABLE_MAX_FILES (sizeof(uint64_t) * 8)
> -
> -struct fdtable {
> -       uint64_t bitmap;
> -       uint32_t fd_start;
> -       struct vfscore_file *files[FDTABLE_MAX_FILES];
> -};
> -struct fdtable fdtable;
> -
> -int vfscore_alloc_fd(void)
> +/* --- stdout ---*/
> +static ssize_t stdout_write(struct vfscore_file *vfscore_file, const void *buf,
> +                            size_t count)
>   {
> -       int ret = ukarch_find_lsbit(~fdtable.bitmap);
> -
> -       if (!ret)
> -               return ret;
> -
> -       fdtable.bitmap |= (uint64_t) 1 << ret;
> -
> -       return ret;
> +       (void) vfscore_file;
> +       return ukplat_coutk(buf, count);
>   }
>   
> -void vfscore_put_fd(int fd)
> -{
> -       UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       /* Currently it is not allowed to free std(in|out|err) */
> -       UK_ASSERT(fd > 2);
> +static struct vfscore_fops stdout_fops = {
> +       .write = stdout_write,
> +};
>   
> -	fdtable.bitmap ^= (uint64_t) 1 << fd;
> -}
> +static struct vfscore_file  stdout_file = {
> +       .fd = 1,
> +       .fops = &stdout_fops,
> +};
>   
> -void vfscore_install_fd(int fd, struct vfscore_file *file)
> -{
> -       UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       UK_ASSERT(fd > 2);
> -       UK_ASSERT(file);
>   
> -	file->fd = fd;
> -       fdtable.files[fd] = file;
> +/* --- stderr ---*/
> +static ssize_t stderr_write(struct vfscore_file *vfscore_file, const void *buf,
> +                            size_t count)
> +{
> +       (void) vfscore_file;
> +       return ukplat_coutk(buf, count);
>   }
You could use the same function for stdout_write and stderr_write (e.g.,
static kern_write()). Then you could avoid this duplication.

>   
> -struct vfscore_file *vfscore_get_file(int fd)
> -{
> -       UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
> -       UK_ASSERT(fd > 2);
> +static struct vfscore_fops stderr_fops = {
> +       .write = stderr_write,
> +};
static struct vfscore_fops kern_fops?

>   
> -	if (!(fdtable.bitmap & ((uint64_t) 1 << fd)))
> -               return NULL;
> +static struct vfscore_file  stderr_file = {
> +       .fd = 1,
> +       .fops = &stderr_fops,
> +};
Of course, he you are going to need 2 (stdout & stderr).

>   
> -	return fdtable.files[fd];
> -}
>   
> -__constructor static void fdtable_init(void)
> +void init_stdio(void)
>   {
> -       memset(&fdtable, 0, sizeof(fdtable));
> -
> -       /* reserve stdin, stdout and stderr */
> -       fdtable.bitmap = 7;
> +       vfscore_install_fd(1, &stdout_file);
> +       vfscore_install_fd(2, &stderr_file);
>   }
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel