Skip to content
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

Add support for QNX Neutrino 7.1 #2055

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

flba-eb
Copy link

@flba-eb flba-eb commented Jun 16, 2023

This adds support for QNX Neutrino 7.1.

@flba-eb flba-eb changed the title Ready for review: Add support for QNX Neutrino 7.1 Add support for QNX Neutrino 7.1 Jun 25, 2023
@flba-eb flba-eb marked this pull request as ready for review June 25, 2023 18:28
@flba-eb flba-eb force-pushed the add_qnx_support_cleaned branch 2 times, most recently from c9ced62 to 3e79a22 Compare June 26, 2023 12:27
README.md Outdated Show resolved Hide resolved
src/errno.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated
@@ -297,12 +304,12 @@ fn readlink_maybe_at<P: ?Sized + NixPath>(
cstr.as_ptr(),
v.as_mut_ptr() as *mut c_char,
v.capacity() as size_t,
),
) as _,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as _ can mask a wide range of type errors. Could you please not do that?

Copy link
Author

@flba-eb flba-eb Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, readlink returns an int on QNX Neutrino. Any suggestion on how to resolve this (maybe one that is also applicable to src/sys/select.rs, see https://github.com/nix-rust/nix/pull/2055/files#r1265967722)?

One possibility (similar to the existing redox handling could be):

    path.with_nix_path(|cstr| unsafe {
        match dirfd {
            #[cfg(target_os = "redox")]
            Some(_) => unreachable!(),
            #[cfg(target_os = "nto")]
            Some(dirfd) => libc::readlinkat(
                dirfd,
                cstr.as_ptr(),
                v.as_mut_ptr() as *mut c_char,
                v.capacity() as size_t,
            ) as libc::ssize_t,
            #[cfg(not(any(target_os = "redox", target_os = "nto")))]
            Some(dirfd) => libc::readlinkat(
                dirfd,
                cstr.as_ptr(),
                v.as_mut_ptr() as *mut c_char,
                v.capacity() as size_t,
            ),
            None => libc::readlink(
                cstr.as_ptr(),
                v.as_mut_ptr() as *mut c_char,
                v.capacity(),
            ),
        }
    })

But this makes the code a bit hard to read. Would a small OS specific wrapper function help? This could look like this diff against master:

diff --git a/src/fcntl.rs b/src/fcntl.rs
index d799ff3..0104bec 100644
--- a/src/fcntl.rs
+++ b/src/fcntl.rs
@@ -282,6 +289,26 @@ fn wrap_readlink_result(mut v: Vec<u8>, len: ssize_t) -> Result<OsString> {
     Ok(OsString::from_vec(v.to_vec()))
 }
 
+#[cfg(target_os = "nto")]
+#[inline(always)]
+unsafe fn readlinkat_wrapper(dirfd: c_int,
+    pathname: *const c_char,
+    buf: *mut c_char,
+    bufsiz: size_t) -> ssize_t
+{
+    libc::readlinkat(dirfd, pathname, buf, bufsiz) as ssize_t
+}
+
+#[cfg(not(target_os = "nto"))]
+#[inline(always)]
+unsafe fn readlinkat_wrapper(dirfd: c_int,
+    pathname: *const c_char,
+    buf: *mut c_char,
+    bufsiz: size_t) -> ssize_t
+{
+    libc::readlinkat(dirfd, pathname, buf, bufsiz)
+}
+
 fn readlink_maybe_at<P: ?Sized + NixPath>(
     dirfd: Option<RawFd>,
     path: &P,
@@ -292,7 +319,7 @@ fn readlink_maybe_at<P: ?Sized + NixPath>(
             #[cfg(target_os = "redox")]
             Some(_) => unreachable!(),
             #[cfg(not(target_os = "redox"))]
-            Some(dirfd) => libc::readlinkat(
+            Some(dirfd) => readlinkat_wrapper(
                 dirfd,
                 cstr.as_ptr(),
                 v.as_mut_ptr() as *mut c_char,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either of those options would work, but I kind of like the first one better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed, please check.

src/sys/resource.rs Outdated Show resolved Hide resolved
let res = match timeout {
Some(t) => {
let mut timeout = t.clone();
unsafe {libc::pselect(nfds, readfds, writefds, errorfds, timeout.as_mut() as *mut libc::timespec, sigmask)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug in Neutrino. POSIX says that the timeout argument should be const. Frankly, given how complicated this makes Nix's code, I don't think it's worth it for the benefit this provides. I would rather just not bind pselect on this platform.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As also described in my previous comment, there are API differences that I'm trying to work around as I cannot change the libc API. Would a small wrapper function help (see other comment)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make it work, but it would definitely ugly-up the code. And this const-mut conversion is worse than the size widening of readlink. Do we know that there are even any QNX users who want to use pselect?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkhilTThomas what do you think about removing pselect or do you know if it is used?

@SteveLauC SteveLauC mentioned this pull request Jan 6, 2024
3 tasks
@AkhilTThomas
Copy link

@flba-eb Hey! Is this PR stuck? would like to have nto support for nix.

@SteveLauC
Copy link
Member

Hi @AkhilTThomas, please see this comment: tokio-rs/tokio#6421 (comment)

@AkhilTThomas
Copy link

Hi @AkhilTThomas, please see this comment: tokio-rs/tokio#6421 (comment)

Awesome thanks!

@flba-eb
Copy link
Author

flba-eb commented Mar 30, 2024

@flba-eb Hey! Is this PR stuck? would like to have nto support for nix.

Adding QNX support to nix is still on my wish-list -- but I'm currently not needing nix in my projects, which is why it didn't get much attention.
I will have more time after the Embedded World (in about two weeks) and can continue, but feel free to "take over" this PR. Maybe it makes sense to fix this together?

@AkhilTThomas
Copy link

@flba-eb Hey! Is this PR stuck? would like to have nto support for nix.

Adding QNX support to nix is still on my wish-list -- but I'm currently not needing nix in my projects, which is why it didn't get much attention. I will have more time after the Embedded World (in about two weeks) and can continue, but feel free to "take over" this PR. Maybe it makes sense to fix this together?

That would be great! I will contact you over email.

@flba-eb flba-eb force-pushed the add_qnx_support_cleaned branch 4 times, most recently from e6c6d7c to c208568 Compare March 31, 2024 13:46
@flba-eb
Copy link
Author

flba-eb commented Mar 31, 2024

Some tests are failing, does anybody have any idea why? On my machine other tests fail also with revision 4ab23c3 (current head on master).

@AkhilTThomas
Copy link

AkhilTThomas commented Mar 31, 2024

Some tests are failing, does anybody have any idea why? On my machine other tests fail also with revision 4ab23c3 (current head on master).

Can you try this patch file. With this all the test cases pass
nix_port_tested.patch

@SteveLauC SteveLauC mentioned this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants