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

[Felipe Huici <Felipe.Huici@xxxxxxxxx>]

Hi Costin,

I'lll give this a reviewed-by, but please read my comment below.

-- Felipe

Reviewed-by: Felipe Huici <felipe.huici@xxxxxxxxx>

On 14.10.19, 16:44, "Costin Lupu" <costin.lupu@xxxxxxxxx> wrote:

    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.

Ok, then as a compromise I suggest that we:

1. Compile-guard the one liner, something like:
#if CONFIG_HAVE_LIBC_TIME /* or CONFIG_UKTIME */
#include <uk/time_types.h>
#endif

and

2. Have noblic's Config.uk imply uktime.

To speed things along, I've given this patch a reviewed-by, but please send 
this fix out sometime this week so we don't lose track of the issue.

-- Felipe
    
    Cheers,
    Costin
    

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