[PATCH 0/5] Fix redefinition errors for toolstack libs

[PATCH 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c                   | 4 ++++
 tools/debugger/kdd/kdd.c                       | 4 ++++
 tools/libfsimage/ext2fs/fsys_ext2fs.c          | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c      | 2 ++
 tools/libs/foreignmemory/private.h             | 6 ++++--
 tools/libs/gnttab/linux.c                      | 6 ++++++
 tools/ocaml/libs/xc/xenctrl_stubs.c            | 8 ++++++++
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 ++++
 tools/ocaml/libs/xl/xenlight_stubs.c           | 4 ++++
 9 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in
/usr/include/limits.h header) then gcc will trigger a redefinition error
because of -Werror. This commit also protects PAGE_SHIFT definitions for
keeping consistency.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/debugger/kdd/kdd-xen.c | 4 ++++
 tools/debugger/kdd/kdd.c     | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..04d2361ba7 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,8 +48,12 @@
 
 #define MAPSIZE 4093 /* Prime */
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT 12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE (1U << PAGE_SHIFT)
+#endif
 
 struct kdd_guest {
     struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..acd5832714 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -288,8 +288,12 @@ static void kdd_log_pkt(kdd_state *s, const char *name, kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT (12)
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE (1ULL << PAGE_SHIFT) 
+#endif
 
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
                                   uint32_t len, void *buf)
-- 
2.20.1

[PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c     | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC      0xEF53	/* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO              2	/* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX                1024	/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT             5	/* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 0170000) == 0040000)
 #define S_ISLNK(mode) (((mode) & 0170000) == 0120000)
 
+#ifndef PATH_MAX
 #define PATH_MAX       1024	/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT    5	/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

[PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in
/usr/include/limits.h header) then gcc will trigger a redefinition error
because of -Werror. This commit also protects PAGE_SHIFT and PAGE_MASK
definitions for keeping consistency.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/libs/foreignmemory/private.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 1ee3626dd2..f3c1ba2867 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -10,11 +10,13 @@
 #include <xen/xen.h>
 #include <xen/sys/privcmd.h>
 
-#ifndef PAGE_SHIFT /* Mini-os, Yukk */
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT           12
 #endif
-#ifndef __MINIOS__ /* Yukk */
+#ifndef PAGE_SIZE
 #define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK            (~(PAGE_SIZE-1))
 #endif
 
-- 
2.20.1

[PATCH 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in
/usr/include/limits.h header) then gcc will trigger a redefinition error
because of -Werror. This commit also protects PAGE_SHIFT and PAGE_MASK
definitions for keeping consistency.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/libs/gnttab/linux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..e12f2697a5 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -36,9 +36,15 @@
 
 #include "private.h"
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT           12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK            (~(PAGE_SIZE-1))
+#endif
 
 #define DEVXEN "/dev/xen/"
 
-- 
2.20.1

[PATCH 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

If PAGE_SIZE is already defined in the system (e.g. in
/usr/include/limits.h header) then gcc will trigger a redefinition error
because of -Werror. This commit also protects PAGE_SHIFT and PAGE_MASK
definitions for keeping consistency.

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already define in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c            | 8 ++++++++
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 ++++
 tools/ocaml/libs/xl/xenlight_stubs.c           | 4 ++++
 3 files changed, 16 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..1c82e76b19 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,22 @@
 
 #include "mmap_stubs.h"
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT		12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE               (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK               (~(PAGE_SIZE-1))
+#endif
 
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
 	((Field(array, index) == Val_none) ? NULL : String_val(Field(Field(array, index), 0)))
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

Re: [PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Julien Grall]

Hi Costin,

On 27/04/2021 13:05, Costin Lupu wrote:
>   tools/libs/foreignmemory/private.h | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
> index 1ee3626dd2..f3c1ba2867 100644
> --- a/tools/libs/foreignmemory/private.h
> +++ b/tools/libs/foreignmemory/private.h
> @@ -10,11 +10,13 @@
>   #include <xen/xen.h>
>   #include <xen/sys/privcmd.h>
>   
> -#ifndef PAGE_SHIFT /* Mini-os, Yukk */
> +#ifndef PAGE_SHIFT
>   #define PAGE_SHIFT           12
>   #endif
> -#ifndef __MINIOS__ /* Yukk */
> +#ifndef PAGE_SIZE
>   #define PAGE_SIZE            (1UL << PAGE_SHIFT)
> +#endif
> +#ifndef PAGE_MASK
>   #define PAGE_MASK            (~(PAGE_SIZE-1))
>   #endif

Looking at the usage, I believe PAGE_* are referring to the page 
granularity of Xen rather than the page granularity of the control 
domain itself.

So it would be incorrect to use the domain's page granularity here and 
would break dom0 using 64KB page granularity on Arm.

Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are 
still referring to the control domain granularity, then we should use 
getpageshift() (Or equivalent) because the userspace shouldn't rely on a 
specific page granularity.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Julien Grall]

On 27/04/2021 13:05, Costin Lupu wrote:
> If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
> header) then gcc will trigger a redefinition error because of -Werror.
> 
> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
> ---
>   tools/libfsimage/ext2fs/fsys_ext2fs.c     | 2 ++
>   tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
>   2 files changed, 4 insertions(+)
> 
> diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c b/tools/libfsimage/ext2fs/fsys_ext2fs.c
> index a4ed10419c..5ed8fce90e 100644
> --- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
> +++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
> @@ -278,7 +278,9 @@ struct ext4_extent_header {
>   
>   #define EXT2_SUPER_MAGIC      0xEF53	/* include/linux/ext2_fs.h */
>   #define EXT2_ROOT_INO              2	/* include/linux/ext2_fs.h */
> +#ifndef PATH_MAX
>   #define PATH_MAX                1024	/* include/linux/limits.h */
> +#endif

Can we drop it completely and just rely on limits.h?

>   #define MAX_LINK_COUNT             5	/* number of symbolic links to follow */
>   
>   /* made up, these are pointers into FSYS_BUF */
> diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c b/tools/libfsimage/reiserfs/fsys_reiserfs.c
> index 916eb15292..10ca657476 100644
> --- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
> +++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
> @@ -284,7 +284,9 @@ struct reiserfs_de_head
>   #define S_ISDIR(mode) (((mode) & 0170000) == 0040000)
>   #define S_ISLNK(mode) (((mode) & 0170000) == 0120000)
>   
> +#ifndef PATH_MAX
>   #define PATH_MAX       1024	/* include/linux/limits.h */
> +#endif
>   #define MAX_LINK_COUNT    5	/* number of symbolic links to follow */
>   
>   /* The size of the node cache */
> 

-- 
Julien Grall

Re: [PATCH 0/5] Fix redefinition errors for toolstack libs

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]



On 27 Apr 2021, at 13:05, Costin Lupu <costin.lupu@cs.pub.ro<mailto:costin.lupu@cs.pub.ro>> wrote:

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Costin Lupu (5):
 tools/debugger: Fix PAGE_SIZE redefinition error
 tools/libfsimage: Fix PATH_MAX redefinition error
 tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
 tools/libs/gnttab: Fix PAGE_SIZE redefinition error
 tools/ocaml: Fix redefinition errors

tools/debugger/kdd/kdd-xen.c                   | 4 ++++
tools/debugger/kdd/kdd.c                       | 4 ++++
tools/libfsimage/ext2fs/fsys_ext2fs.c          | 2 ++
tools/libfsimage/reiserfs/fsys_reiserfs.c      | 2 ++
tools/libs/foreignmemory/private.h             | 6 ++++--
tools/libs/gnttab/linux.c                      | 6 ++++++
tools/ocaml/libs/xc/xenctrl_stubs.c            | 8 ++++++++
tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 ++++
tools/ocaml/libs/xl/xenlight_stubs.c           | 4 ++++
9 files changed, 38 insertions(+), 2 deletions(-)

—
2.20.1


For the OCaml bindings, this avoids redefinitions as you say. Looks good to me.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 2962 bytes --]

Re: [PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 4/28/21 12:03 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 27/04/2021 13:05, Costin Lupu wrote:
>>   tools/libs/foreignmemory/private.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libs/foreignmemory/private.h
>> b/tools/libs/foreignmemory/private.h
>> index 1ee3626dd2..f3c1ba2867 100644
>> --- a/tools/libs/foreignmemory/private.h
>> +++ b/tools/libs/foreignmemory/private.h
>> @@ -10,11 +10,13 @@
>>   #include <xen/xen.h>
>>   #include <xen/sys/privcmd.h>
>>   -#ifndef PAGE_SHIFT /* Mini-os, Yukk */
>> +#ifndef PAGE_SHIFT
>>   #define PAGE_SHIFT           12
>>   #endif
>> -#ifndef __MINIOS__ /* Yukk */
>> +#ifndef PAGE_SIZE
>>   #define PAGE_SIZE            (1UL << PAGE_SHIFT)
>> +#endif
>> +#ifndef PAGE_MASK
>>   #define PAGE_MASK            (~(PAGE_SIZE-1))
>>   #endif
> 
> Looking at the usage, I believe PAGE_* are referring to the page
> granularity of Xen rather than the page granularity of the control
> domain itself.
> 
> So it would be incorrect to use the domain's page granularity here and
> would break dom0 using 64KB page granularity on Arm.
> 
> Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are
> still referring to the control domain granularity, then we should use
> getpageshift() (Or equivalent) because the userspace shouldn't rely on a
> specific page granularity.

Yes, this makes sense. One thing I need to clarify: what does XC stand
for? I thought for some time it stands for Xen Control.

Thanks,
Costin

Re: [PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

On 4/28/21 12:04 PM, Julien Grall wrote:
> 
> 
> On 27/04/2021 13:05, Costin Lupu wrote:
>> If PATH_MAX is already defined in the system (e.g. in
>> /usr/include/limits.h
>> header) then gcc will trigger a redefinition error because of -Werror.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
>> ---
>>   tools/libfsimage/ext2fs/fsys_ext2fs.c     | 2 ++
>>   tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> index a4ed10419c..5ed8fce90e 100644
>> --- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> +++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> @@ -278,7 +278,9 @@ struct ext4_extent_header {
>>     #define EXT2_SUPER_MAGIC      0xEF53    /* include/linux/ext2_fs.h */
>>   #define EXT2_ROOT_INO              2    /* include/linux/ext2_fs.h */
>> +#ifndef PATH_MAX
>>   #define PATH_MAX                1024    /* include/linux/limits.h */
>> +#endif
> 
> Can we drop it completely and just rely on limits.h?
> 

One problem here is that the system limits.h header doesn't necessarily
include linux/limits.h, which would mean we would have to include
linux/limits.h. But this is problematic for other systems such as BSD.

I had a look on a FreeBSD source tree to see how this is done there. It
seems that there are lots of submodules, apps and libs that redefine
PATH_MAX in case it wasn't defined before so the changes introduced by
the current patch seem to be very popular. Another clean approach I saw
was for jemalloc [1] which includes unistd.h. They redefine PATH_MAX
only for MS C compiler, but AFAIK we don't need that.

So IMHO the current changes seem to be the most portable, but I'm open
to any suggestions.

[1]
https://github.com/jemalloc/jemalloc/blob/dev/include/jemalloc/internal/jemalloc_internal_decls.h#L76


>>   #define MAX_LINK_COUNT             5    /* number of symbolic links
>> to follow */
>>     /* made up, these are pointers into FSYS_BUF */
>> diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> b/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> index 916eb15292..10ca657476 100644
>> --- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> +++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> @@ -284,7 +284,9 @@ struct reiserfs_de_head
>>   #define S_ISDIR(mode) (((mode) & 0170000) == 0040000)
>>   #define S_ISLNK(mode) (((mode) & 0170000) == 0120000)
>>   +#ifndef PATH_MAX
>>   #define PATH_MAX       1024    /* include/linux/limits.h */
>> +#endif
>>   #define MAX_LINK_COUNT    5    /* number of symbolic links to follow */
>>     /* The size of the node cache */
>>
> 

Re: [PATCH 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

On 4/28/21 3:34 PM, Christian Lindig wrote:
> 
> 
>> On 27 Apr 2021, at 13:05, Costin Lupu <costin.lupu@cs.pub.ro
>> <mailto:costin.lupu@cs.pub.ro>> wrote:
>>
>> For replication I used gcc 10.3 on an Alpine system. In order to
>> replicate the
>> redefinition error for PAGE_SIZE one should install the 'fortify-headers'
>> package which will change the chain of included headers by indirectly
>> including
>> /usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.
>>
>> Costin Lupu (5):
>>  tools/debugger: Fix PAGE_SIZE redefinition error
>>  tools/libfsimage: Fix PATH_MAX redefinition error
>>  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
>>  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
>>  tools/ocaml: Fix redefinition errors
>>
>> tools/debugger/kdd/kdd-xen.c                   | 4 ++++
>> tools/debugger/kdd/kdd.c                       | 4 ++++
>> tools/libfsimage/ext2fs/fsys_ext2fs.c          | 2 ++
>> tools/libfsimage/reiserfs/fsys_reiserfs.c      | 2 ++
>> tools/libs/foreignmemory/private.h             | 6 ++++--
>> tools/libs/gnttab/linux.c                      | 6 ++++++
>> tools/ocaml/libs/xc/xenctrl_stubs.c            | 8 ++++++++
>> tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 ++++
>> tools/ocaml/libs/xl/xenlight_stubs.c           | 4 ++++
>> 9 files changed, 38 insertions(+), 2 deletions(-)
>>
>> —
>> 2.20.1
>>
> 
> For the OCaml bindings, this avoids redefinitions as you say. Looks good
> to me.
> 
> Acked-by: Christian Lindig <christian.lindig@citrix.com
> <mailto:christian.lindig@citrix.com>>
> 

Thanks, Christian, I'll add your ack on the Ocaml patch for the next
version of the series.

Cheers,
Costin

Re: [PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Julien Grall]

On 28/04/2021 19:27, Costin Lupu wrote:
> Hi Julien,

Hi Costin,

> On 4/28/21 12:03 PM, Julien Grall wrote:
>> Hi Costin,
>>
>> On 27/04/2021 13:05, Costin Lupu wrote:
>>>    tools/libs/foreignmemory/private.h | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/libs/foreignmemory/private.h
>>> b/tools/libs/foreignmemory/private.h
>>> index 1ee3626dd2..f3c1ba2867 100644
>>> --- a/tools/libs/foreignmemory/private.h
>>> +++ b/tools/libs/foreignmemory/private.h
>>> @@ -10,11 +10,13 @@
>>>    #include <xen/xen.h>
>>>    #include <xen/sys/privcmd.h>
>>>    -#ifndef PAGE_SHIFT /* Mini-os, Yukk */
>>> +#ifndef PAGE_SHIFT
>>>    #define PAGE_SHIFT           12
>>>    #endif
>>> -#ifndef __MINIOS__ /* Yukk */
>>> +#ifndef PAGE_SIZE
>>>    #define PAGE_SIZE            (1UL << PAGE_SHIFT)
>>> +#endif
>>> +#ifndef PAGE_MASK
>>>    #define PAGE_MASK            (~(PAGE_SIZE-1))
>>>    #endif
>>
>> Looking at the usage, I believe PAGE_* are referring to the page
>> granularity of Xen rather than the page granularity of the control
>> domain itself.
>>
>> So it would be incorrect to use the domain's page granularity here and
>> would break dom0 using 64KB page granularity on Arm.
>>
>> Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are
>> still referring to the control domain granularity, then we should use
>> getpageshift() (Or equivalent) because the userspace shouldn't rely on a
>> specific page granularity.
> 
> Yes, this makes sense. One thing I need to clarify: what does XC stand
> for? I thought for some time it stands for Xen Control.

I think it is Xen Control, which is a bit confusing for that specific 
define.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Julien Grall]

Hi Costin,

On 28/04/2021 19:35, Costin Lupu wrote:
> On 4/28/21 12:04 PM, Julien Grall wrote:
>>
>>
>> On 27/04/2021 13:05, Costin Lupu wrote:
>>> If PATH_MAX is already defined in the system (e.g. in
>>> /usr/include/limits.h
>>> header) then gcc will trigger a redefinition error because of -Werror.
>>>
>>> Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
>>> ---
>>>    tools/libfsimage/ext2fs/fsys_ext2fs.c     | 2 ++
>>>    tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
>>>    2 files changed, 4 insertions(+)
>>>
>>> diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>>> b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>>> index a4ed10419c..5ed8fce90e 100644
>>> --- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>>> +++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>>> @@ -278,7 +278,9 @@ struct ext4_extent_header {
>>>      #define EXT2_SUPER_MAGIC      0xEF53    /* include/linux/ext2_fs.h */
>>>    #define EXT2_ROOT_INO              2    /* include/linux/ext2_fs.h */
>>> +#ifndef PATH_MAX
>>>    #define PATH_MAX                1024    /* include/linux/limits.h */
>>> +#endif
>>
>> Can we drop it completely and just rely on limits.h?
>>
> 
> One problem here is that the system limits.h header doesn't necessarily
> include linux/limits.h, which would mean we would have to include
> linux/limits.h. But this is problematic for other systems such as BSD.

That's annoying :).

> 
> I had a look on a FreeBSD source tree to see how this is done there. It
> seems that there are lots of submodules, apps and libs that redefine
> PATH_MAX in case it wasn't defined before so the changes introduced by
> the current patch seem to be very popular. Another clean approach I saw
> was for jemalloc [1] which includes unistd.h. They redefine PATH_MAX
> only for MS C compiler, but AFAIK we don't need that.

I am not aware of anyone using MS C compiler to build the tools.

> 
> So IMHO the current changes seem to be the most portable, but I'm open
> to any suggestions.

Right, this is the good thing of your approach. I can't see a better 
solution if the system limits.h doesn't always define PATH_MAX. So:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Tim Deegan]

Hi,

At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
> If PAGE_SIZE is already defined in the system (e.g. in
> /usr/include/limits.h header) then gcc will trigger a redefinition error
> because of -Werror. This commit also protects PAGE_SHIFT definitions for
> keeping consistency.

Thanks for looking into this!  I think properly speaking we should fix
this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
kdd-xen.c.  If for some reason we ever ended up with a system-defined
PAGE_SIZE that wasn't 4096u then we would not want to use it here
because it would break our guest operations.

Cheers,

Tim

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Tim,

On 4/29/21 10:58 PM, Tim Deegan wrote:
> Hi,
> 
> At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
>> If PAGE_SIZE is already defined in the system (e.g. in
>> /usr/include/limits.h header) then gcc will trigger a redefinition error
>> because of -Werror. This commit also protects PAGE_SHIFT definitions for
>> keeping consistency.
> 
> Thanks for looking into this!  I think properly speaking we should fix
> this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
> those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
> kdd-xen.c.  If for some reason we ever ended up with a system-defined
> PAGE_SIZE that wasn't 4096u then we would not want to use it here
> because it would break our guest operations.

As discussed for another patch of the series, an agreed solution that
would apply for other libs as well would be to use XC_PAGE_* macros
instead of PAGE_* macros. I've just sent a v2 doing that. Please let me
know if you think it would be better to introduce the KDD_PAGE_*
definitions instead.

Cheers,
Costin

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Tim Deegan]

At 14:36 +0300 on 30 Apr (1619793419), Costin Lupu wrote:
> Hi Tim,
> 
> On 4/29/21 10:58 PM, Tim Deegan wrote:
> > Hi,
> > 
> > At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
> >> If PAGE_SIZE is already defined in the system (e.g. in
> >> /usr/include/limits.h header) then gcc will trigger a redefinition error
> >> because of -Werror. This commit also protects PAGE_SHIFT definitions for
> >> keeping consistency.
> > 
> > Thanks for looking into this!  I think properly speaking we should fix
> > this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
> > those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
> > kdd-xen.c.  If for some reason we ever ended up with a system-defined
> > PAGE_SIZE that wasn't 4096u then we would not want to use it here
> > because it would break our guest operations.
> 
> As discussed for another patch of the series, an agreed solution that
> would apply for other libs as well would be to use XC_PAGE_* macros
> instead of PAGE_* macros. I've just sent a v2 doing that. Please let me
> know if you think it would be better to introduce the KDD_PAGE_*
> definitions instead.

Sorry to be annoying, but yes, please do introduce the KDD_ versions.
All the xen-specific code in KDD lives in kdd-xen.c; kdd.c shouldn't
include any xen headers.

Cheers,

Tim.

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

On 4/30/21 9:45 PM, Tim Deegan wrote:
> At 14:36 +0300 on 30 Apr (1619793419), Costin Lupu wrote:
>> Hi Tim,
>>
>> On 4/29/21 10:58 PM, Tim Deegan wrote:
>>> Hi,
>>>
>>> At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
>>>> If PAGE_SIZE is already defined in the system (e.g. in
>>>> /usr/include/limits.h header) then gcc will trigger a redefinition error
>>>> because of -Werror. This commit also protects PAGE_SHIFT definitions for
>>>> keeping consistency.
>>>
>>> Thanks for looking into this!  I think properly speaking we should fix
>>> this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
>>> those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
>>> kdd-xen.c.  If for some reason we ever ended up with a system-defined
>>> PAGE_SIZE that wasn't 4096u then we would not want to use it here
>>> because it would break our guest operations.
>>
>> As discussed for another patch of the series, an agreed solution that
>> would apply for other libs as well would be to use XC_PAGE_* macros
>> instead of PAGE_* macros. I've just sent a v2 doing that. Please let me
>> know if you think it would be better to introduce the KDD_PAGE_*
>> definitions instead.
> 
> Sorry to be annoying, but yes, please do introduce the KDD_ versions.
> All the xen-specific code in KDD lives in kdd-xen.c; kdd.c shouldn't
> include any xen headers.

No worries, will do. I imagined that might be the case for kdd.c, but I
wasn't sure.

Cheers,
Costin