Re: [Minios-devel] [UNIKRAFT/NEWLIB PATCH 1/1] Add sys/types.h to the mman header

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Vlad, Felipe,

hold on a second. I think that patch is going a bit too far. If you 
include sys/types.h, sys/mman.h will provide (indirectly) a lot more 
types than it's supposed to do according to the standard. That can be a 
problem if your code relies on that: for example, using pid_t without 
including sys/types.h directly. That would work with that patch, but if 
you move to a different libc implementation, your code would suddenly 
break in unexpected ways because it can't find pid_t any more.
I noticed that the current code has __NEED macros, which is something 
that is used in musl (and that we adapted for nolibc). However, this is 
not how newlib does it. Instead, at least from by superficial 
understanding of newlib, they create underscore variants of data types, 
and then typedef them over as needed.
So instead of including sys/types.h, one should include sys/_types.h, 
and then do the definitions for off_t, mode_t, and size_t:
#ifndef _OFF_T_DECLARED
typedef __off_t off_t;
#define _OFF_T_DECLARED
#endif

(and the others accordingly.) That's how I see it some of newlib's code 
do it, for example in newlib/libc/sys/rtems/include/sys/mman.h.
I also see __NEED macros in some other include files in the newlib glue 
code, so we might have to fix those sooner rather than later, too, 
because I don't think they're doing anything.
Cheers,
Florian


On 3/26/19 1:47 PM, Felipe Huici wrote:
> Hi Vlad,
>
> Thanks for the patch, looks good. Just a few minor grammar things in the commit 
> message which I'll fix on upstreaming.
>
> — Felipe
>
> Reviewed-by: Felipe Huici <felipe.huici@xxxxxxxxx>
>
> On 26.03.19, 13:16, "Vlad-Andrei BĂDOIU (78692)" 
> <vlad_andrei.badoiu@xxxxxxxxxxxxxxx> wrote:
>
>      The mman.h header is take from musl but
>      it does not include the bits/alltypes.h
>      and such off_t and size_t are not defined.
>      This patch adds the sys/types.h header
>      which has the missing typedefs.
>      
>      Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
>      ---
>       include/sys/mman.h | 1 +
>       1 file changed, 1 insertion(+)
>      
>      diff --git a/include/sys/mman.h b/include/sys/mman.h
>      index 7abf57f..0981a4b 100644
>      --- a/include/sys/mman.h
>      +++ b/include/sys/mman.h
>      @@ -31,6 +31,7 @@ extern "C" {
>       #endif
>       
>       #include <stddef.h>
>      +#include <sys/types.h>
>       
>       #define __NEED_mode_t
>       #define __NEED_size_t
>      --
>      2.20.1
>      
>      
--
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