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

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:

> 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 ;-)
Sorry, that is a brand new git :). I actually disabled it, apparently
did not go completely away. But if you look at v2, you will see that it
was way worth :).

>
>> 
>> 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).
This was actually multiple copy-paste errors. It is supposed to be coutk
and coutd. But as you proposed, I re-implemented it with only coutk - so
duplicated code is gone in the v4.
>
>>   
>> -    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);
>>   }
>> 

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel