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

[Julien Grall <julien@xen.org>]

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