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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 10/15/19 9:52 AM, Felipe Huici wrote:
> 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.

I've just sent the patch, it's called ' lib/nolibc: Imply uktime for
time functions'

Costin

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