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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxx>
> > > ---
> > >    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@xxxxxxxxxx>

Cheers,

--
Julien Grall