New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RISC-V: Use v2 syscalls when appropriate #2
Conversation
bdb75f6
to
6d247cb
Compare
The RISC-V port does not support any legacy syscalls.
6d247cb
to
cd95681
Compare
| @@ -6,7 +6,11 @@ int rename(const char *old, const char *new) | |||
| { | |||
| #ifdef SYS_rename | |||
| return syscall(SYS_rename, old, new); | |||
| #else | |||
| #ifdef SYS_renameat2 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible typo: This one uses the new syscall if both old and new exist, but the mmap/renameat changes prefer the old one.
(I'm not a musl or riscv maintainer, I'm just a random nobody who found this PR through your blog https://drewdevault.com/2018/12/20/Porting-Alpine-Linux-to-RISC-V.html. Hi.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye. I took the more conservative approach with this one based on the ifdefs which were already here - they prefer rename over renameat even though the latter is newer/more flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth leaving a comment in the code here to that effect? Anytime something is “surprising” (as the commenter was) usually qualifies IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mention this when it comes up in code review - but since I didn't write the original code, I don't think it's appropriate for me to speculate on the intentions of the author.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, SYS_renameat needs to be used rather than SYS_renameat2 if both are defined on the arch, since there exist old kernels were SYS_renameat2 is not present (ENOSYS). These changes would make musl stop working on those kernels. Also, from a style perspective, #elif defined(... is probably preferable to avoid gratuitously nested #ifdef (harder to read). These changes are not riscv-specific but necessary for support of any new arch where the old syscall isn't present, and should be merged upstream as prerequisites for the riscv port.
|
A generalized version of this patch has been submitted upstream. |
new mount api syscalls were added, same numers on all targets, see linux commit a07b20004793d8926f78d63eb5980559f7813404 vfs: syscall: Add open_tree(2) to reference or clone a mount linux commit 2db154b3ea8e14b04fee23e3fdfd5e9d17fbc6ae vfs: syscall: Add move_mount(2) to move mounts around linux commit 24dcb3d90a1f67fe08c68a004af37df059d74005 vfs: syscall: Add fsopen() to prepare for superblock creation linux commit ecdab150fddb42fe6a739335257949220033b782 vfs: syscall: Add fsconfig() for configuring and managing a context linux commit 93766fbd2696c2c4453dd8e1070977e9cd4e6b6d vfs: syscall: Add fsmount() to create a mount for a superblock linux commit cf3cba4a429be43e5527a3f78859b1bfd9ebc5fb vfs: syscall: Add fspick() to select a superblock for reconfiguration linux commit 9c8ad7a2ff0bfe58f019ec0abc1fb965114dde7d uapi, x86: Fix the syscall numbering of the mount API syscalls [ver #2] linux commit d8076bdb56af5e5918376cd1573a6b0007fc1a89 uapi: Wire up the mount API syscalls on non-x86 arches [ver #2]
The RISC-V port does not support any legacy syscalls.