Re: [Minios-devel] [UNIKRAFT PATCH 2/3] lib/uktime: Introduce time_types.h for type defintions

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 10/14/19 5:02 PM, Felipe Huici wrote:
> Hi Costin,
> 
> Please find a comment inline.
> 
> -- Felipe
> 
> 
> On 13.10.19, 15:30, "Costin Lupu" <costin.lupu@xxxxxxxxx> wrote:
> 
>     We basically take the type definitions we used for nolibc and put them in
>     time_types.h.
>     
>     In this patch we also add the changes for allowing custom definitions in 
> libc
>     implementations. We allow for all libc implementations but for nolibc; the
>     alternative would have been to add empty headers in nolibc.
>     
>     Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>     ---
>      .../include/nolibc-internal/shareddefs.h      | 41 +---------
>      lib/uktime/include/uk/time_types.h            | 74 +++++++++++++++++++
>      lib/uktime/musl-imported/include/sys/time.h   | 10 ++-
>      lib/uktime/musl-imported/include/time.h       |  7 ++
>      plat/xen/x86/arch_time.c                      |  2 +-
>      5 files changed, 91 insertions(+), 43 deletions(-)
>      create mode 100644 lib/uktime/include/uk/time_types.h
>     
>     diff --git a/lib/nolibc/include/nolibc-internal/shareddefs.h 
> b/lib/nolibc/include/nolibc-internal/shareddefs.h
>     index 9eb613a1..a1661081 100644
>     --- a/lib/nolibc/include/nolibc-internal/shareddefs.h
>     +++ b/lib/nolibc/include/nolibc-internal/shareddefs.h
>     @@ -65,46 +65,7 @@ typedef __off off_t;
>      #define __DEFINED_off_t
>      #endif
>      
>     -#if (defined __NEED_time_t && !defined __DEFINED_time_t)
>     -typedef long time_t;
>     -#define __DEFINED_time_t
>     -#endif
>     -
>     -#if (defined __NEED_suseconds_t && !defined __DEFINED_suseconds_t)
>     -typedef long suseconds_t;
>     -#define __DEFINED_suseconds_t
>     -#endif
>     -
>     -#if (defined __NEED_struct_timeval && !defined __DEFINED_struct_timeval)
>     -struct timeval {
>     - time_t      tv_sec;
>     - suseconds_t tv_usec;
>     -};
>     -#define __DEFINED_struct_timeval
>     -#endif
>     -
>     -#if (defined __NEED_struct_timespec && !defined 
> __DEFINED_struct_timespec)
>     -struct timespec {
>     - time_t tv_sec;
>     - long   tv_nsec;
>     -};
>     -#define __DEFINED_struct_timespec
>     -#endif
>     -
>     -#if defined(__NEED_timer_t) && !defined(__DEFINED_timer_t)
>     -typedef void *timer_t;
>     -#define __DEFINED_timer_t
>     -#endif
>     -
>     -#if (defined __NEED_clockid_t && !defined __DEFINED_clockid_t)
>     -typedef int clockid_t;
>     -#define __DEFINED_clockid_t
>     -#endif
>     -
>     -#if defined(__NEED_clock_t) && !defined(__DEFINED_clock_t)
>     -typedef long clock_t;
>     -#define __DEFINED_clock_t
>     -#endif
>     +#include <uk/time_types.h>
> 
> This sets a hard dependency for nolibc on uktime, which shouldn't be the 
> case. You should probably compile-guard it -- we'd like to be able to keep 
> noblic as small as possible where applicable.

You're right, if we want to be able to disable time-related functions
from nolibc then this include directive should be protected. But that's
not enough, we should also make some kind of relationship between nolibc
and uktime in order to avoid things to get messy (please see my reply on
the `lib/vfscore: Add dependency to libuktime` thread).

More than that, the include guard should check whether the "time
features" are enabled - this is a more generic way than checking whether
uktime is enabled. So we might need to define a CONFIG_HAVE_LIBC_TIME
flag which will be set when enabling time libs such as uktime.

Cheers,
Costin

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