unistd/dir: fix resolve_path behavior on trailing slashes #346
Conversation
Unit Test Results7 155 tests ±0 6 440 ✅ - 8 38m 7s ⏱️ + 3m 57s For more details on these failures, see this check. Results for commit a3a6f07. ± Comparison against base commit 8e7daf8. ♻️ This comment has been updated with latest results. |
unistd/dir.c
Outdated
| @@ -229,6 +231,12 @@ static int _resolve_abspath(char *path, char *result, int resolve_last_symlink, | |||
| } | |||
| } | |||
|
|
|||
| if ((is_leaf != 0) && (resolve_last_symlink == 0)) { | |||
| /* TODO: in this case it's more efficient to check if file exists | |||
There was a problem hiding this comment.
- a little bit misleading comment, you probably meant "instead of"? (as
_readlink_abswould fail fast if lookup fails; still we do needgetModeto check if it's a dir or not?) IMHO not worth a comment as the proposed solution is invalid (it's not a hot path, let's not dwell on it) - not sure if the issue is solved completely, what would happen in case of
/symlink/to/file/(when resolve_last_symlink == 0, eg.lstat). Is this handled correctly by callers?
There was a problem hiding this comment.
- Checking if path exists and is a directory before call to
_readlink_absmay be slightly more efficient in the case when(is_leaf != 0) && (resolve_last_symlink == 0)and the symlink exists. With this code the symlink's contents will be read even though it's not necessary. It's probably not a big performance concern. - You're right, when doing
lstaton/path/to/symlink/, ifsymlinkexists and is a symlink then the call will succeed despite the trailing slash. I think in this case we need to checkif (ends_with_slash != 0)and exit witherrno == -ENOTDIR.
There was a problem hiding this comment.
- Checking if path exists and is a directory before call to
_readlink_absmay be slightly more efficient in the case when(is_leaf != 0) && (resolve_last_symlink == 0)and the symlink exists. With this code the symlink's contents will be read even though it's not necessary. It's probably not a big performance concern.
still, this should be instead of, not before as in the mentioned case you don't want to call _readlink_abs at all (if we really want to improve performance). I think it's not worth the additional code complexity (not a hot path), so IMHO TODO should be removed
Regarding (2) -> maybe add a test for this particular "corner case" to avoid regressions in the future (this code is hard enough to understand with all special cases ;))
unistd/dir.c
Outdated
| return SET_ERRNO(-EINVAL); | ||
| if (!S_ISLNK(msg.o.attr.val)) { | ||
| SET_ERRNO(-EINVAL); | ||
| return S_ISDIR(msg.o.attr.val) ? -EISDIR : -1; |
There was a problem hiding this comment.
ugly API (returning side info as an error). It's our internal function, maybe add explicit output param (it would increase the readability of the caller code)?
There was a problem hiding this comment.
I think it's a good idea, I'll change the API a bit to do this more elegantly.
|
Please also copy the problem description from PR to the commit message body (or write something similar there to leave a trail) |
jmaksymowicz
force-pushed
the
jmaksymowicz/RTOS-782
branch
from
1689d8c
to
a3a6f07
Compare
Description
Currently, any part of the system that uses the
resolve_pathfunction (includingstat,lstat,realpathand many others) don't consider the trailing slashes at the end of the path. For directories this is correct behavior, but if the last component of a path is not a directory, then the function should return with error and seterrnotoENOTDIR.The change currently breaks CI, as one of the tests for
pshchecks that trailing slashes for files are ignored.Motivation and Context
Closes phoenix-rtos/phoenix-rtos-project#682 and brings API more in line with Open Group specification
Types of changes
This change may break existing software (including
pshscripts) if it depends on the current trailing slash behavior. This may be difficult to detect if a path is generated automatically.How Has This Been Tested?
Checklist:
Special treatment