subsys: fs: Support file open flags to fs and POSIX API

[de-nordic]

{DNM] Due to required update to mcumgr, that is reviewed here: apache/mynewt-mcumgr#90
and requires update of https://github.com/zephyrproject-rtos/mcumgr

Anyone that is able to review apache/mynewt-mcumgr#90 please do so!

This series of commits adds support for file open flags (CREATE, APPEND, etc.) and fs_open_ex function to support them.
POSIX API was updated accordingly.

• FAT FS tests
• LIttleFS test
• Posix tests

RFC Issue: #26833
Resolves: #16638

Requires changes to mcumgr: apache/mynewt-mcumgr#90
Requires changes to mcumgr, for the regression to pass, brought by snapshot update: zephyrproject-rtos/mcumgr#26

[jimparis]

POSIX is an API specification, so "adding to posix API" does not make sense, because then it's not posix anymore. I think we should instead fix open() (by not unconditionally adding O_CREAT) and not introduce open2() at all.

[jimparis]

I think we should fix open() (by not unconditionally adding O_CREAT) and not introduce open2() at all.

[jimparis]

If I'm understanding you right, you are saying that littlefs requires flags to be either LFS_O_RDONLY, LFS_O_WRONLY, or LFS_O_RDWR, and passing a flag value of 0 is treated the same as LFS_O_RDWR. If that's true, shouldn't it be the job of lfs_flags_from_zephyr to never pass a flag value of 0 to lfs?

[de-nordic]

lfs_flags_from_zephyr is supposed to translate flags to obtain, from fs backend, specific behavior. I guess that the READONLY would be closest to expected.

[de-nordic]

POSIX is an API specification, so "adding to posix API" does not make sense, because then it's not posix anymore. I think we should instead fix open() (by not unconditionally adding O_CREAT) and not introduce open2() at all.

I am not sure I can do that immediately because someone may be using current implementation the way it is. If it just gets changed then the code will stop working. Maybe there should be some Kconfig added that would enable compatibility with the original behavior for some time.

[jimparis]

I dislike FILE_SYSTEM_LEGACY_OPEN. I suggest using some macro tricks like the ones described at https://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros, which will let users continue to pass either 2 or 3 arguments to fs_open, so that both existing code and new code can coexist and work correctly. For example, this test appears to do the trick:

#include <stdio.h>

#define FO_READ   0x01
#define FO_WRITE  0x02
#define FO_RDWR   (FO_READ | FO_WRITE)
#define FO_CREATE 0x10

void real_fs_open(void *p, const char *path, int flags) {
        printf("open(p=%p, path=\"%s\", flags=0x%02x)\n", p, path, flags);
}

void legacy_fs_open(void *p, const char *path) {
        return real_fs_open(p, path, FO_CREATE | FO_RDWR);
}

/* With 0-2 arguments, fs_open(...) resolves to legacy_fs_open(...)
   With 3   arguments, fs_open(...) resolves to real_fs_open(...)
   More arguments are not handled.
*/
#define GET_ARG4(arg1, arg2, arg3, arg4, ...) arg4
#define OPEN_CHOOSE(...) GET_ARG4(                                      \
                __VA_ARGS__,  real_fs_open,                             \
                legacy_fs_open, legacy_fs_open, legacy_fs_open)
#define fs_open(...) OPEN_CHOOSE(__VA_ARGS__)(__VA_ARGS__)

/* Use example: */
main()
{
        char p;

        fs_open(&p, "/no-flags");
        fs_open(&p, "/with-flags", FO_READ);

        return 0;
}

Output:

$ gcc -Wall -pedantic -o test test.c
$ ./test
open(p=0x7fff85ee440f, path="/no-flags", flags=0x13)
open(p=0x7fff85ee440f, path="/with-flags", flags=0x01)

As for the POSIX side, I would just fix it and not support the old broken behavior at all. If it's still made configurable, I would strongly suggest making it default to the correct open() API; otherwise it doesn't make sense to call it POSIX at all, and I am certain that anyone attempting to port POSIX code to Zephyr would welcome the fix.

[de-nordic]

I dislike FILE_SYSTEM_LEGACY_OPEN. I suggest using some macro tricks (...)

OK I can do that. I just hoped to deprecate the old function signature with time and leave the 3 parameter thing only.

As for the POSIX side, I would just fix it and not support the old broken behavior at all (...)

I do not know how many users have been using this in the form it is. The "legacy" behavior is supposed to be deprecated after some time. The programmers should be given time to adjust code. If we will keep on breaking their applications without giving time to adjust to changes, they will be reluctant to update Zephyr within their projects.

[jimparis]

I dislike FILE_SYSTEM_LEGACY_OPEN. I suggest using some macro tricks (...)

OK I can do that. I just hoped to deprecate the old function signature with time and leave the 3 parameter thing only.

That sounds good. You could use the macro and mark the old signature with __attribute__((deprecated)) so GCC will warn if it gets used.

As for the POSIX side, I would just fix it and not support the old broken behavior at all (...)

I do not know how many users have been using this in the form it is. The "legacy" behavior is supposed to be deprecated after some time. The programmers should be given time to adjust code. If we will keep on breaking their applications without giving time to adjust to changes, they will be reluctant to update Zephyr within their projects.

I don't think it's a big deal either way, but I would still learn towards fixing it because:

• the POSIX API is clear
• the open() function signature isn't changing; i.e. users have always had to pass flags to it
• Previously, the flags were just ignored
• Now, the flags are interpreted correctly

So basically the only case where someone would get bitten by the change is if they were previously passing flags that didn't match the behavior they actually wanted -- ie. if they were passing "O_RDONLY" and then expecting to be able to write the file.

I would suggest naming the configuration option POSIX_FS_LEGACY_OPEN_IGNORE_FLAGS with a description like:

With this option set, the POSIX open() call will ignore the user-supplied flags, and act as if O_CREAT|O_RDWR were passed. For backwards compatibility with Zephyr < A.B.C. This option will default to n starting in Zephyr X.Y.Z.

Unfortunately, because open() already took flags, it's harder to help the user out with compile-time warnings about the upcoming deprecation. Maybe use __builtin_constant_p in a macro to issue a warning if the passed flags are something other than O_CREAT|O_RDWR, with the legacy option? It wouldn't warn for non-constant flags, but should still cover some use cases.

[pabigot]

One style change, one spelling fix.

[de-nordic]

@pabigot Thanks!

This is no longer relevant because I has been decided to drop compatibility with two parameter ffs_open.

[carlescufi]

Use cfe5eb98a9493017448846fd1a44a9340bd0a22f