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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

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.

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 */
>>
>