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
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
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
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
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
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
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
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
[-- 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 --]
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
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 */ >> >
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
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
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
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
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
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.
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