Re: [Minios-devel] [UNIKRAFT PATCH v3 10/23] lib/vfscore: add utility funcs&defs to support imported

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi,

Sharan Santhanam <sharan.santhanam@xxxxxxxxx> writes:

> Hello yuri,
>
> The patch seems functionally fine.
>
> There are some minor issue. Please find them inline.
>
> Thanks & Regards
> Sharan
>
> On 2/7/19 2:58 PM, Yuri Volchkov wrote:
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>>   lib/vfscore/include/vfscore/file.h  |  2 ++
>>   lib/vfscore/include/vfscore/fs.h    | 28 ++++++++++++++++++++++++
>>   lib/vfscore/include/vfscore/vnode.h | 21 ++++++++++++++++++
>>   lib/vfscore/main.c                  | 33 +++++++++++++++++++++++++++++
>>   lib/vfscore/mount.c                 | 17 +++++++++++++++
>>   lib/vfscore/vfs.h                   |  9 ++++++++
>>   lib/vfscore/vnode.c                 |  2 ++
>>   7 files changed, 112 insertions(+)
>>   create mode 100644 lib/vfscore/include/vfscore/fs.h
>> 

>> diff --git a/lib/vfscore/include/vfscore/fs.h 
>> b/lib/vfscore/include/vfscore/fs.h
>> new file mode 100644
>> index 00000000..1bb4e24d
>> --- /dev/null
>> +++ b/lib/vfscore/include/vfscore/fs.h
>> @@ -0,0 +1,28 @@
>> +#ifndef _VFSCORE_FS_H_
>> +#define _VFSCORE_FS_H_
>> +
>> +#include <fcntl.h>
>> +/*
>> + * Kernel encoding of open mode; separate read and write bits that are
>> + * independently testable: 1 greater than the above.
>> + */
>> +#define FREAD           0x00000001
>> +#define FWRITE          0x00000002
>> +
>> +#define ALLPERMS (S_ISUID|S_ISGID|S_ISVTX|S_IRWXU|S_IRWXG|S_IRWXO)
>> +
>
> Tab instead of spaces.
Sorry I do not get the comment.

>> diff --git a/lib/vfscore/main.c b/lib/vfscore/main.c
>> index 82e45917..1164b30b 100644
>> --- a/lib/vfscore/main.c
>> +++ b/lib/vfscore/main.c
>> @@ -52,8 +52,41 @@
>>   int        vfs_debug = VFSDB_FLAGS;
>>   #endif
>>   
>> +/* This macro is for defining an alias of the 64bit version of a
>> + * syscall to the regular one. It seams we can make the logic which is
>> + * choosing the right call simpler then in common libc.
>> + *
>> + * Let's keep LFS64 calls just in case in future we will find out that
>> + * these aliases are need.
>> + */
>> +#define LFS64(x)
>> +
>>   static mode_t global_umask = S_IWGRP | S_IWOTH;
>>   
>> +/* TODO: these macro does not belong here
>> + * NOTE: borrowed from OSv
>> + */
>> +#define DO_ONCE(thing) do {                         \
>> +    static int _x;                                  \
>> +    if (!_x) {                                      \
>> +        _x = 1;                                     \
>> +        thing ;                                     \
>> +    }                                               \
>> +} while (0)
>> +#define WARN_STUBBED() DO_ONCE(uk_pr_warn("%s() stubbed\n", __func__))
>> +
>
> This macro makes assumption on the signature of the function.
> But since this is accessible only within the main.c it is fine.
Correct. This is purely internal thing for main.c. And it is made
specifically for stubbing syscalls, which do have same signature.

>> @@ -79,6 +79,23 @@ fs_getfs(const char *name)
>>      return fs;
>>   }
>>   
>> +int device_open(const char *name, int mode, struct device **devp)
>> +{
>> +    (void) name;
>> +    (void) mode;
>> +    (void) devp;
> We use the __unused attribute everywhere instead of (void).
I did not like __unused because it is polluting the function
parameters. But you are right, I will change this to __unused to keep
the code consistent

>
>> +
>> +    uk_pr_err("device open is not implemented (%s)\n", name);
>> +    return 0;
> Shouldn't we report an error for not implemented functions.

Answering to this and the next question, the return value of
device_(open|close) is not checked in the imported code. I don't really
want to modify this, because we do not have a concept of the devices
yet, and this branch of vfscore just yet to be implemented. Together
with the devices.

That is why I think crashing is the right thing to do, since we do not
really know what to do in such a case.

>
>> +}
>> +
>> +int device_close(struct device *dev)
>> +{
>> +    (void) dev;
>> +    UK_CRASH("not implemented");
> Why are crashing for not implementing functions here?
>
>> +    return 0;
>> +}
>> +
>>   int
>>   sys_mount(const char *dev, const char *dir, const char *fsname, int flags, 
>> const void *data)
>>   {
>> diff --git a/lib/vfscore/vfs.h b/lib/vfscore/vfs.h
>> index 19f8ac87..609dc1f5 100644
>> --- a/lib/vfscore/vfs.h
>> +++ b/lib/vfscore/vfs.h
>> @@ -151,4 +151,13 @@ void     vnode_dump(void);
>>   void        mount_dump(void);
>>   #endif
>>   
>> +static void __attribute__((unused)) uk_vfscore_trace(int foo __unused, ...)
>> +{
>> +}
>> +
>> +#define TRACEPOINT(trace_name, fmt, ...)                    \
>> +    static void trace_name(__VA_ARGS__ ) __attribute__((unused, 
>> alias("uk_vfscore_trace")))
>> +
>> +
>> +
>>   #endif /* !_VFS_H */
>> diff --git a/lib/vfscore/vnode.c b/lib/vfscore/vnode.c
>> index a2f45596..86e6d9c7 100644
>> --- a/lib/vfscore/vnode.c
>> +++ b/lib/vfscore/vnode.c
>> @@ -48,6 +48,8 @@
>>   #include <vfscore/vnode.h>
>>   #include "vfs.h"
>>   
>> +#define S_BLKSIZE 512
>> +
>>   enum vtype iftovt_tab[16] = {
>>      VNON, VFIFO, VCHR, VNON, VDIR, VNON, VBLK, VNON,
>>      VREG, VNON, VLNK, VNON, VSOCK, VNON, VNON, VBAD,
>> 

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