fs: Clarify description of fs_open when no access bits given #64146
Conversation
|
FYI @clamattia |
de-nordic
force-pushed
the
fix-for-64030
branch
from
8882d3a to
bba1ffa
Compare
|
@kartben you have convinced me, with the "makeshift and error-prone", to take your change... partially, as I have removed the suggested usage to check for file existence. |
:) |
There was a problem hiding this comment.
What about making it clear from the start:
* If @p flags are set to 0 the function will attempt to open file with
* no read/write access. This should not be used to check if a file
* exists as this will generate a log error when the file is not found
* and requires the file to be closed when it is present. Instead use
* fs_stat to check if a file exists.
No, I have taken the warning part but removed suggestion for using fs_open to check if file exists at all. Edit: Yeah, wrong branch pushed.... eh. Monday every day. |
de-nordic
force-pushed
the
fix-for-64030
branch
from
bba1ffa to
4dc3f12
Compare
I, unfortunately, find this usage to be a hack. I know that this works but if the usage would be intended we would have to silence the error for that specific case. Additionally, if the file exists you have to actually close it afterwards, which is another possibility for mistake and leaving open handlers. We still need to fix some test cases that provide no flags, when checking whether they can open a file. |
include/zephyr/fs/fs.h
Outdated
| * If @p flags are set to 0 the function will attempt to open an existing file | ||
| * with no read/write access; this may be used to e.g. check if the file exists. | ||
| * @warning If @p flags are set to 0 the function will attempt to open file with | ||
| * no read/write access. |
There was a problem hiding this comment.
Why does this have a @warning ? It seems to be an allowed usage pattern but why does it have a warning,
There was a problem hiding this comment.
Because you will open file but will not be able to read/write to it.
Maybe. Because backends may interpret some of open flags differently, for example create may imply write on one backend, but not on an another.
There was a problem hiding this comment.
The problem I have with the @warning is that (without some extra information) the user does not know what is warned against.
There was a problem hiding this comment.
I don't know, seems that the info no lack of read/write access is enough.
There was a problem hiding this comment.
I agree, but when adding the @warning it should warn against something. Either remove the warning or add something like: When issued on a existing file the file needs to be closed afterwards. When issued on a non-existing file an error message will be generated
There was a problem hiding this comment.
I do not want to mention error message, because any subsystem called by driver may actually render error messages, or not depending on Kconfig, so it is pointless to provide such message here.
de-nordic
force-pushed
the
fix-for-64030
branch
from
4dc3f12 to
db7dd6b
Compare
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
@de-nordic What is the state on this? Was it decided, that it is not needed? |
Add warning that file opened without R/W flags will have no read/write access. Remove suggestion for using fs_open to check if file exists. Clarify -ENOENT return reason. Fixes zephyrproject-rtos#64030 Signed-off-by: Dominik Ermel <[email protected]>
de-nordic
force-pushed
the
fix-for-64030
branch
from
4e62988 to
32e0724
Compare
|
@kartben I have taken in your Nit. |
Can you take a look again? |
|
This should go into 3.6 as it is a documentation improvement. |
Thanks! No problem. |
Added note warnign that fs_open should not be used for checking whether file exists or not and fs_stat should be used instead.
Fixes #64030