Re: [Minios-devel] [UNIKRAFT PATCH v2 4/6] lib/ramfs: adapt imported ramfs to Unikraft

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi Simon,


Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:

> Hey Yuri,
>
> please find my comments inline.
>
> Thanks,
>
> Simon
>
>> On 8. Feb 2019, at 15:31, Yuri Volchkov <yuri.volchkov@xxxxxxxxx> wrote:
>> 
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>> lib/Config.uk            |   1 +
>> lib/Makefile.uk          |   1 +
>> lib/ramfs/Config.uk      |   4 +
>> lib/ramfs/Makefile.uk    |   5 ++
>> lib/ramfs/exportsyms.uk  |   1 +
>> lib/ramfs/ramfs.h        |   4 +-
>> lib/ramfs/ramfs_vfsops.c |  28 ++++---
>> lib/ramfs/ramfs_vnops.c  | 171 +++++++++++++++++++++------------------
>> 8 files changed, 124 insertions(+), 91 deletions(-)
>> create mode 100644 lib/ramfs/Config.uk
>> create mode 100644 lib/ramfs/Makefile.uk
>> create mode 100644 lib/ramfs/exportsyms.uk
>> 
>> diff --git a/lib/ramfs/Config.uk b/lib/ramfs/Config.uk
>> new file mode 100644
>> index 00000000..52e4388c
>> --- /dev/null
>> +++ b/lib/ramfs/Config.uk
>> @@ -0,0 +1,4 @@
>> +config LIBRAMFS
>> +    bool "ramfs: simple RAM file system"
>> +    default n
>> +    depends on LIBVFSCORE
>> diff --git a/lib/ramfs/Makefile.uk b/lib/ramfs/Makefile.uk
>> new file mode 100644
>> index 00000000..cd6dafd5
>> --- /dev/null
>> +++ b/lib/ramfs/Makefile.uk
>> @@ -0,0 +1,5 @@
>> +$(eval $(call addlib_s,libramfs,$(CONFIG_LIBRAMFS)))
>> +
>> +LIBRAMFS_SRCS-y += $(LIBRAMFS_BASE)/ramfs_vfsops.c
>> +LIBRAMFS_SRCS-y += $(LIBRAMFS_BASE)/ramfs_vnops.c
>> +LIBRAMFS_CFLAGS += -DDEBUG_RAMFS -DUK_DEBUG
>
> I would not handover UK_DEBUG always. See also my comments on patch 5.
> A menu option in Config.uk could be added to introduce this flag when 
> selected. 
That is a mistake. It has sneaked from the debug code. I am killing this
line in the v3. As for menu option, let's consider it later, when
DPRINTF is settled.

>
>> diff --git a/lib/ramfs/exportsyms.uk b/lib/ramfs/exportsyms.uk
>> new file mode 100644
>> index 00000000..c86c3f35
>> --- /dev/null
>> +++ b/lib/ramfs/exportsyms.uk
>> @@ -0,0 +1 @@
>> +none
>> \ No newline at end of file
>> diff --git a/lib/ramfs/ramfs.h b/lib/ramfs/ramfs.h
>> index 01e4da09..91fb579c 100644
>> --- a/lib/ramfs/ramfs.h
>> +++ b/lib/ramfs/ramfs.h
>> @@ -33,7 +33,7 @@
>> #ifndef _RAMFS_H
>> #define _RAMFS_H
>> 
>> -#include <osv/prex.h>
>> +#include <vfscore/prex.h>
>> 
>> /* #define DEBUG_RAMFS 1 */
>> 
>> @@ -61,7 +61,7 @@ struct ramfs_node {
>>      struct timespec rn_atime;
>>      struct timespec rn_mtime;
>>      int rn_mode;
>> -    bool rn_owns_buf;
>> +    int rn_owns_buf;
>
> We have `stdbool.h` in the meantime. You do not need to replace the bool 
> datatype anymore.
Ok, I restored the original boolian code for ramfs.

>> };
>> 
>> struct ramfs_node *ramfs_allocate_node(const char *name, int type);
>> @@ -345,7 +352,7 @@ ramfs_truncate(struct vnode *vp, off_t length)
>>                      np->rn_buf = NULL;
>>                      np->rn_bufsize = 0;
>>              }
>> -    } else if (size_t(length) > np->rn_bufsize) {
>> +    } else if ((size_t) length > np->rn_bufsize) {
>>              // XXX: this could use a page level allocator
>
> You could convert this XXX comment to a TODO. We have actually a page level 
> allocator interface that we could use in the future.
XXX means same as TODO. But ok, I can change that.

>
>>              new_size = round_page(length);
>>              new_buf = malloc(new_size);
>

-- 
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