stdio/file: fix ftell_unlocked use of lseek

[julianuziemblo]

When the file mode is O_APPEND and we're writing, we have to seek to the end of the file backing the stream.
This PR also handles all places where we should exit the writing mode, e.g. fflush, fseek, etc.

Tangently-related to phoenix-rtos/phoenix-rtos-project#1403, as a test disabled by this issue was not passing due to this bug.

YT: RTOS-1474

Description

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: enable tests disabled by phoenix-rtos/phoenix-rtos-project#1403 phoenix-rtos-tests#523
• Tested by hand on: (list targets here).

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work (list the PRs, preferably in merge-order).
• I will merge this PR by myself when appropriate.

[gemini-code-assist]

Code Review

This pull request modifies ftell_unlocked in stdio/file.c to conditionally use SEEK_END or SEEK_CUR based on whether the stream is in append mode and has buffered data. However, the current condition stream->bufpos != stream->bufeof is problematic as it can cause incorrect offsets during read operations or when transitioning from reading to writing. It is recommended to instead check if the stream is in writing mode and has buffered write data.

[github-actions]

Unit Test Results

13 730 tests  ±0   12 882 ✅ +11   58m 7s ⏱️ + 1m 35s
   780 suites ±0      848 💤  - 11 
     1 files   ±0        0 ❌ ± 0 

Results for commit 48c4390. ± Comparison against base commit 9bffcee.

♻️ This comment has been updated with latest results.

[nalajcie]

this should probably be fixed in kernel, as calling lseek directly on fd will still be invalid? That approach would also hopefully fix the ftell at the same time?

[julianuziemblo]

this should probably be fixed in kernel, as calling lseek directly on fd will still be invalid? That approach would also hopefully fix the ftell at the same time?

This is a fix to buffered writing in append mode, kernel has no idea about buffered I/O. If you mean a fix to concurrent writing in O_APPEND, it's done in other commits, attached to the related phoenix-rtos/phoenix-rtos-project#1403

[nalajcie]

this should probably be fixed in kernel, as calling lseek directly on fd will still be invalid? That approach would also hopefully fix the ftell at the same time?

This is a fix to buffered writing in append mode, kernel has no idea about buffered I/O. If you mean a fix to concurrent writing in O_APPEND, it's done in other commits, attached to the related phoenix-rtos/phoenix-rtos-project#1403

what I'm hinting at - is that's not enough. In other PRs (concurrent appending) You're changing the offs in the FS server on O_APPEND, but the kernel knows only the old / invalid value (lseek(fd, 0, SEEK_CUR) will be invalid).

The proper fix would be to either pass the value back to kernel or make kernel re-read it after updating (or maybe . Implementing it properly on kernel side would make this change not needed (SEEK_CUR would always be synchronized with SEEK_END after writing for O_APPEND files).

I think there are some problems with the files opened with O_RDWR (fpoen("a+")) - you're allowed to seek and read from them so the SEEK_CUR can be different from SEEK_END. One error I see is that fseek flushes the buffer (correctly) but doesn't clear the F_WRITING flag (not the only place it's missing, other functions need also to be reviewed) so your ftell would return invalid offet there (end ot file instead of the set one).

IMHO:

1. Add testcases for valid use cases
2. Fix it kernel-side (update offs + write should be atomic...)
3. Fix in libphoenix the use cases which are currently broken dye to incorrect userspace implementation ("a+", but maybe not only)

[nalajcie]

as per my comment, this will break "a+" streams

[julianuziemblo]

what I'm hinting at - is that's not enough. In other PRs (concurrent appending) You're changing the offs in the FS server on O_APPEND, but the kernel knows only the old / invalid value (lseek(fd, 0, SEEK_CUR) will be invalid).

The proper fix would be to either pass the value back to kernel or make kernel re-read it after updating (or maybe . Implementing it properly on kernel side would make this change not needed (SEEK_CUR would always be synchronized with SEEK_END after writing for O_APPEND files).

yes, I'll be adding this shortly as I came to the same conclussion concurrently 😛

I think there are some problems with the files opened with O_RDWR (fpoen("a+")) - you're allowed to seek and read from them so the SEEK_CUR can be different from SEEK_END. One error I see is that fseek flushes the buffer (correctly) but doesn't clear the F_WRITING flag (not the only place it's missing, other functions need also to be reviewed) so your ftell would return invalid offet there (end ot file instead of the set one).

I will check it more thouroughly then and try to fix the cases and add more tests for them.

[julianuziemblo]

Implementing it properly on kernel side would make this change not needed (SEEK_CUR would always be synchronized with SEEK_END after writing for O_APPEND files).

This is false - doing fwrite $\rightarrow$ ftell on a buffered "a+" stream would not return the proper file offset as we didn't write anything yet - we just buffered it. We have to then manually seek to the end of the file to ensure the proper offset is returned. That's why this change is needed regardless.

[nalajcie]

Implementing it properly on kernel side would make this change not needed (SEEK_CUR would always be synchronized with SEEK_END after writing for O_APPEND files).

This is false - doing fwrite → ftell on a buffered "a+" stream would not return the proper file offset as we didn't write anything yet - we just buffered it. We have to then manually seek to the end of the file to ensure the proper offset is returned. That's why this change is needed regardless.

Ack, still the fix is incomplete without tracking consistently when we have something in the buffer and when not (sometimes it's only F_WRITING, somethimes with bufpos, etc.)