Re: [Minios-devel] [UNIKRAFT PATCH v4 1/9] lib/nolibc: Add type definitions for timer support

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Sharan,


On 08/24/2018 02:45 PM, Sharan Santhanam wrote:
> Hello,
>
> Please find my comment inline.
>
> On 08/20/2018 01:21 PM, Florian Schmidt wrote:
> > Add missing type definition for timer support with nolibc.
> >
> > Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> > Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> > ---
> >   lib/nolibc/include/nolibc-internal/shareddefs.h | 15 +++++++++++++++
> >   lib/nolibc/include/sys/select.h                 |  6 ++++++
> >   lib/nolibc/include/sys/types.h                  |  4 ++++
> >   3 files changed, 25 insertions(+)
> >
> > diff --git a/lib/nolibc/include/nolibc-internal/shareddefs.h 
> > b/lib/nolibc/include/nolibc-internal/shareddefs.h
> > index 9b81fab..8b953c2 100644
> > --- a/lib/nolibc/include/nolibc-internal/shareddefs.h
> > +++ b/lib/nolibc/include/nolibc-internal/shareddefs.h
> > @@ -65,6 +65,11 @@ typedef __off off_t;
> >   #define __DEFINED_off_t
> >   #endif
> > +#if (defined __NEED_sigset_t && !defined __DEFINED_sigset_t)
> According to this posix spec[1], sigset_t should be integer or a 
> structure object.
Actually, after refactoring the code, we don't even need thi definition 
anywhere, so I removed it. (sigset is only used in the linux platform 
code, which needs to provide its own k_sigset_t anyway to ensure ABI 
compatibility with the Linux kernel.) Once we need functions that 
actually use sigset_t, we can choose what data type feels best.
> > +typedef unsigned long sigset_t;
> > +#define __DEFINED_sigset_t
> > +#endif
> > +
> >   #if (defined __NEED_time_t && !defined __DEFINED_time_t)
> >   typedef long time_t;
> >   #define __DEFINED_time_t
> > @@ -90,3 +95,13 @@ struct timespec {
> >   };
> >   #define __DEFINED_struct_timespec
> >   #endif
> > +
> > +#if (defined __NEED_clockid_t && !defined __DEFINED_clockid_t)
> clockid_t is defined as signed long. But since it is an identifier with 
> a handful of values it might be fine.
Same as above.

> > +typedef unsigned long clockid_t;
> Should be __DEFINED_clockid_t instead of __DEFINED_clockid_d.
> > +#define __DEFINED_clockid_d
> > +#endif
> > +
> timer_t Posix specification does not specify the actual type 
> information. But glibc, musl define it as (void *) and in the linux 
> kernel it is defined as (int). We have defined it as unsigned long.
> Why did we make this choice?
Same as above.

> > +#if (defined __NEED_timer_t && !defined __DEFINED_timer_t)
> > +typedef unsigned long timer_t;
> > +#define __DEFINED_timer_t
> > +#endif
> > diff --git a/lib/nolibc/include/sys/select.h 
> > b/lib/nolibc/include/sys/select.h
> > index e2bc70d..01ca905 100644
> > --- a/lib/nolibc/include/sys/select.h
> > +++ b/lib/nolibc/include/sys/select.h
> > @@ -36,6 +36,12 @@
> >   extern "C" {
> >   #endif
> > +#define __NEED_time_t
> > +#define __NEED_suseconds_t
> > +#define __NEED_sigset_t
> > +#define __NEED_struct_timespec
> > +#include <nolibc-internal/shareddefs.h>
> > +
> >   typedef unsigned long __fd_mask;
> >   /*
> > diff --git a/lib/nolibc/include/sys/types.h 
> > b/lib/nolibc/include/sys/types.h
> > index 9908855..f2b6633 100644
> > --- a/lib/nolibc/include/sys/types.h
> > +++ b/lib/nolibc/include/sys/types.h
> > @@ -46,6 +46,10 @@ extern "C" {
> >   #define __NEED_size_t
> >   #define __NEED_ssize_t
> >   #define __NEED_off_t
> > +#define __NEED_time_t
> > +#define __NEED_suseconds_t
> > +#define __NEED_clockid_t
> > +#define __NEED_timer_t
> >   #include <nolibc-internal/shareddefs.h>
> >   #ifdef __cplusplus
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html
> [2] 
> http://pubs.opengroup.org/onlinepubs/009695299/basedefs/sys/types.h.html
> Thanks & Regards
> Sharan
--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

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