Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Struct field naming contention with stdlib C/C++ header files struct_stat.h #defines in Linux. #50

Closed
phelter opened this issue Aug 8, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@phelter
Copy link
Contributor

phelter commented Aug 8, 2023

Describe the bug
When integrating ff_stdio.h with Unix x86_64-linux-gnu builds, there are multiple definitions of:
st_atime, st_mtime, st_ctime - These are defined in /usr/include/x86_64-linux-gnu/bits/struct_stat.h: Line 77-79 as macros:

#  define st_atime st_atim.tv_sec	/* Backward compatibility.  */
#  define st_mtime st_mtim.tv_sec
#  define st_ctime st_ctim.tv_sec

This conflicts with the definition of ff_stdio.h which uses the same name as these for parameters inside FF_Stat_t definition here

If you could please use another name for your time support for internal definitions so that it doesn't conflict with the naming in the stdlibs.

Target

  • Development board: [Linux test environment Ubuntu 22.04]
  • Instruction Set Architecture: N/A - just using API and mocking the implementation.
  • IDE and version: N/A - but using VSCode
  • Toolchain and version: [Clang 14.0.0]

Host

  • Host OS: [Ubuntu 22.04]
  • Version: 22.04

To Reproduce

  • Create a project that includes both ff_stdio.h and includes something that includes struct_stat.h from the c headers.
  • Attempt to compile - will complain with:
[build] In file included from ...ThirdParty/freertos/mock_plus_fat/./mock_freertos_plus_fat.h:7:
[build] .../build/_deps/freertos_plus_fat-src/include/ff_stdio.h:92:20: error: expected ';' at end of declaration list
[build]             time_t st_atime;
[build]                    ^
[build] /usr/include/x86_64-linux-gnu/bits/struct_stat.h:77:27: note: expanded from macro 'st_atime'
[build] #  define st_atime st_atim.tv_sec       /* Backward compatibility.  */
[build]                           ^

Happens for st_atime, st_mtime and st_ctime.

Expected behavior
No naming convention contention with stdlibs of C/C++.

Screenshots
N/A

Wireshark logs
N/A

Additional context
N/A

@phelter phelter added the bug Something isn't working label Aug 8, 2023
@phelter
Copy link
Contributor Author

phelter commented Aug 8, 2023

Just to reiterate - this is an issue only when trying to compile for testing purposes. In a real-life situation would never attempt to use FreeRTOS-Plus-FAT with linux since it already has it's own file system drivers. But for consistency sake would appreciate renaming these values since they clash when used for debugging via cross compiling for native compiles and using the ff_stdio.h file from FreeRTOS-Plus-FAT.

phelter added a commit to phelter/Lab-Project-FreeRTOS-FAT that referenced this issue Aug 8, 2023
@htibosch
Copy link
Contributor

Hi @phelter

In a real-life situation would never attempt to use FreeRTOS-Plus-FAT with linux since it already has it's own file system drivers

I also worked on WIN32 when developing and testing FreeRTOS-Plus-FAT, so why not use it in the Linux port? It can be used as a FAT RAM-disk driver.

Beside that: normally I would be happy to change struct fields, but in this case, many existing applications refer to st_atime and st_ctime.

Can you think of a downward compatible solution? Can we avoid the #define by un-defining __USE_MISC before including the header files:

#ifdef __USE_MISC
    struct timespec st_atim;         /* Time of last access.  */
    #define st_atime st_atim.tv_sec  /* Backward compatibility.  */
#else
#endif

@phelter
Copy link
Contributor Author

phelter commented Aug 12, 2023

@htibosch

Can you think of a downward compatible solution? Can we avoid the #define by un-defining __USE_MISC before including the header files:

No, unfortunately I went through the scenarios I don't see an alternative that is downward compatible. It's unfortunate that the names exist in struct_stat.h - hence the reason when I use #defines for C based libraries I prefix them with some unique name to the library itself.

I don't see this being a huge issue for updating if people are aware of the breaking change.

The only thing that I can think of is if it is Linux - then define FF_STAT in the following manner:

#ifdef (UNIX) // - or whatever port that uses sys/stat.h - might be POSIX based.
    #include <sys/stat.h>
    typedef struct stat FF_STAT;
#else
   // Current definition
#endif

So it's the exact same struct - and there is no duplication of the definition. Otherwise define something that is linux like but not use the same names.

AniruddhaKanhere added a commit that referenced this issue Aug 24, 2023
…f PR#49 (#51)

* Adding in FF_SDDiskInitWithSettings optional initialization to support runtime initial mounting options.

* FAT-#50: Fixing duplicate definition of st_atime, st_mtime, st_ctime for linux.

* Fixing formatting with diff provided using uncrustify.

---------

Co-authored-by: Paul Helter <[email protected]>
@AniruddhaKanhere
Copy link
Member

Since the PR #51 is merged, I shall be closing this issue.

Thank you for bringing this to our attention @phelter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants