Add support for target_os = android#25
Conversation
| let flags = sealing::seals_to_bitflags(seals); | ||
| // UNSAFE(lucab): required syscall. | ||
| let r = unsafe { libc::syscall(libc::SYS_fcntl, fd, libc::F_ADD_SEALS, flags) }; | ||
| let r = unsafe { libc::fcntl(fd, libc::F_ADD_SEALS, flags) }; |
There was a problem hiding this comment.
I recall there being a reason to use the syscall interface. Not sure what it was, however.
There was a problem hiding this comment.
Ok. Do you remember? Using libc::fcntl feel more portable than the explicit sys call name. I replaced syscall with fnctl because I had build errors for aarch64-linux-android with the SYS_fcntl arg which is not exposed by libc. I haven't digged which syscall name to use instead which would also be an option with a cfg(target_os = ... guard.
There was a problem hiding this comment.
It looks like I wrote it that way initially, but I don't remember exactly why. Trying to retro-guess myself, it's likely one of those reasons:
fcntl()is a muxed wrapper in most libc implementations, and I didn't see a real reason to go through an additional opaque layer (and to make writing seccomp filters easier)- I was possibly playing with this on a platform where libc-rs was missing the function binding (uclibc? musl?) and so I went for the syscall directly
- I was playing with the idea of making this work just with sc and without libc-rs at all, but I never walked that path till the end
Or possibly a mix of all of them. The only interesting one nowadays is the first, but portability is likely a better argument.
There was a problem hiding this comment.
@lucab Thanks for your reply. Which way would you like to go? I can try to refactor this patch to use libc::syscall withtarget_os (and maybe target_arch) specific guards. Otherwise we can also check which targets are affected at all.
There was a problem hiding this comment.
@flxo if you are not in a hurry, it may be worth to just dig a bit more into libc-rs. If it is just missing a few SYS_fcntl definitions, it would be good to add it there; they have a self-service release process so it shouldn't add to much delay before being able to consume it here.
Instead, if the reason turns out to be more complex than that and we don't find any blocker for switching to the libc muxed wrapper, we can freely do that as a fallback.
In any case, thanks for looking into this :)
There was a problem hiding this comment.
@lucab I'll have a look why my first attempt with the direct use of libc::syscall didn't work for aarch64-linux-android.
Bump to 0.2.99 I'd like to get a new release published, since I want to make use of #2308 in lucab/memfd-rs#25
Bump to 0.2.99 I'd like to get a new release published, since I want to make use of #2308 in lucab/memfd-rs#25
|
@lucab Hi. I'm back from vacation :-) Looks like the merge of the |
|
@flxo thanks, patch looks fine. It would be good to add an explicit version to the |
Add `target_os = android` to the conditional compilation flags. Add targets `aarch64-linux-android` and `arm-linux-androideabi` with the use of `cross` the the GH action.
|
@lucab Many thanks! Would you mind publishing a 0.4.1? There doesn't seem to be anything in the queue and I'd love to get rid of another |
Android supports memfds. Add
target_os = androidto the conditional compilation flags. Replace the use of raw libc::syscallwith a more genericlibc::fcntl` calls.Test are fine on a AOSP Android 9.0: