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

added sigevent and mq_notify #199

Closed
wants to merge 2 commits into from
Closed

added sigevent and mq_notify #199

wants to merge 2 commits into from

Conversation

MarkusJais
Copy link

I've added support (Linux) for sigevent in order to add mq_notify. I've contributed mq* support for nix-rust and will update it there to use libc. mq_notifiy was missing and more complex because it requires sigevent.

Tests in ci directory (on Ubuntu) did pass but this is my first contribution to libc and should receive careful review.

Happy to help!

pub sigev_value: sigval_t,
pub sigev_signo: ::c_int,
pub sigev_notify: ::c_int,
pub sigev_notify_function: ::dox::Option<extern fn(sigev_value: sigval_t)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This may actually want to just be a usize, C has a habit of sometimes using sentinel values for function pointers that mean "invalid" or "do this other specific action"

@kamalmarhubi
Copy link
Contributor

I can't tell why the musl build is failing. It can't find sigval_t, but I think it's in mqueue.h which is being included?

@alexcrichton
Copy link
Member

Thanks! Looks like there's a few CI errors as well for platforms like MUSL

@MarkusJais
Copy link
Author

I'll try to have a look at the MUSL library.

Part of the bindings are basically what rust-bindgen generated but with better names. I am not sure if bindgen can properly handle unions.

I don't think sigevent is particularly useful or even a good idea (low level, probably error prone with threads and signals) but I'll try to make it work as it should be available to give Rust programmers all the options of the C API (that doesn't mean that I encourage using it for IPC, there are better options).

@alexcrichton
Copy link
Member

Yeah for now it's ok to leave it as opaque and we can fill it out over time if necessary

@MarkusJais
Copy link
Author

BTW, are there any guidelines on how to handle C unions in Rust (and in libc)? I couldn't find that much searching the web.

@alexcrichton
Copy link
Member

Unfortunately not really. One of the only unions we deal with today is siginfo and it's just dealt with a little ad-hocly. Without language support for unions there's not a lot we can do here unless the C side has a few extra structures for each version of the union.

Basically it's case-by-case today.

@MarkusJais
Copy link
Author

In that case, is it even possible to get this done in a safe way? If unions cannot be dealt with at compile time it might be hard to get the memory layout and size correct.
I am not compiler expert but I fear it could end up in broken memory.

@kamalmarhubi
Copy link
Contributor

There's an RFC for adding unions, which looks like it's going places. Until then, I think it's ad hoc code downstream from libc has to be careful to do things right. There's an RFC for bitfields too, but it has a bit less activity so far.

To help with getting downstream code right, I came across the idea of having a small C fragment to ensure correct endianness / ABI handling. I think I'll be trying that out this week.

@alexcrichton
Copy link
Member

For now it's probably best to get the size/alignment correct and not worry about the fields for now. That way it's at least a usable API and the fields can be extracted manually downstream.

@MarkusJais
Copy link
Author

Wow, that RFC has a lot of comments. Obviously not a trivial problem :-)
The size right now was generated with bindgen. I had all tests run on my machine and they were ok but I haven't yet looked in musl. Will do.

@kamalmarhubi
Copy link
Contributor

The size right now was generated with bindgen. I had all tests run on my machine and they were ok but I haven't yet looked in musl. Will do.

Since musl is using the same syscall ABI, the types should have the same size and layout. I took a quick look at musl, and the problem is that the union isn't typedefed as sigval_t:

union sigval {
    int sival_int;
    void *sival_ptr;
};

http://git.musl-libc.org/cgit/musl/tree/include/signal.h#n91

vs same in Linux

typedef union sigval {
        int sival_int;
        void __user *sival_ptr;
} sigval_t;

http://lxr.free-electrons.com/source/include/uapi/asm-generic/siginfo.h#L7

@kamalmarhubi
Copy link
Contributor

Based on this, I think the only thing that can be done is to skip the type in libc-test. Is that right @alexcrichton?

@alexcrichton
Copy link
Member

I'd rather not skip types as cases like this seems tricky, perhaps this could just be an opaque struct for now?

@MarkusJais
Copy link
Author

maybe a stupid question but what do you mean with "opaque struct"?

I agree about not skipping tests!

@kamalmarhubi
Copy link
Contributor

I believe it means a struct with private fields that have the right size and alignment, but not necessarily the right types. So, opaque in the sense that you can have a value of it, but cannot inspect its contents in any way.

@MarkusJais
Copy link
Author

After reading more about unions and Rust I believe it is best to wait until Rust has support for unions (see the RFC mentioned above). Otherwise it seems to be too fragile to add more unions.
I hope the union support mentioned in that RFC will be part of Rust soon.

@alexcrichton
Copy link
Member

Closing this due to inactivity, but the good news is that we've accepted an RFC for union and that'll hopefully be implemented in the near future!

@MarkusJais
Copy link
Author

cool, looking forward to the union support.

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.

3 participants