| Message ID | e44209c8981a68604a34f3066d53989f84ce8f49.1619524463.git.costin.lupu@cs.pub.ro (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix redefinition errors for toolstack libs | expand |
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 */ >
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 */ >> >
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,
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 */
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(+)