Conditionally support SYNC flags

[zoraaver]

To make it clearer to users when synchronization behaviour is not supported, return ENOTSUP when O_RSYNC, O_DSYNC or O_SYNC are respectively not defined. Linux also doesn't support O_RSYNC despite the O_RSYNC flag being defined.

[yamt]

It's not yet clear whether these flags should be added to the standard so disallow use of them for the time being. See WebAssembly/WASI#748 for more details. These changes are also necessary to ensure the rust WASI tests pass since they assert that path_open returns ENOTSUP when passing O_DSYNC/O_RSYNC/O_SYNC.

it sounds like a bug in tests to me.
preview1 has these flags defined.

[lum1n0us]

It also bothers me. They are defined but not used in wasi-libc till ec4566beae84e54952637f0bf61bee4b4cacc087.

[yamt]

It also bothers me. They are defined but not used in wasi-libc till ec4566beae84e54952637f0bf61bee4b4cacc087.

what do you mean by "not used in wasi-libc"?
doesn't open() pass-through them to wasi?

[yamt]

WebAssembly/wasi-testsuite#91

[zoraaver]

it sounds like a bug in tests to me.

I don't think it's that clear-cut. I'm not suggesting that this change is definitive or the correct approach but for the time being I would say it's better to explicitly not support these flags rather than offering half-baked support depending on the underlying OS. e.g. Linux doesn't actually support O_RSYNC.

The purpose of this change is mainly so we can start running the rust tests in CI, not to spark a discussion on the use of these flags.

If you want to merge WebAssembly/wasi-testsuite#91, that also works but it made more sense to me personally to remove support for these flags in WAMR.

@wenyongh do you have any further thoughts? This is a breaking change so would require mentioning in the release notes if I'm not mistaken.

[wenyongh]

If you want to merge WebAssembly/wasi-testsuite#91, that also works but it made more sense to me personally to remove support for these flags in WAMR.

@wenyongh do you have any further thoughts? This is a breaking change so would require mentioning in the release notes if I'm not mistaken.

How about making these flags optional, just let the developer choose whether to support it or not? By default they are unsupported, and we add a macro to control the code and mention the change in the release notes?

[zoraaver]

Thanks, that sounds like a good idea. I'll amend this PR to add a build flag to control whether these flags are supported or not so developers can opt in if they require it for their use cases.

[yamt]

do you mean something like CONFIG_HAS_ macros in ssp_config.h?

[yamt]

do you mean something like CONFIG_HAS_ macros in ssp_config.h?

i didn't notice you have already updated the patch when asking this.
anyway, to me, it seems more appropriate to use CONFIG_HAS_ style macros than user-facing config knobs.

[wenyongh]

LGTM

[loganek]

Looks good, thanks