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.

Sign up for GitHub

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

Jump to bottom

SockProtocol: No support for redundant protocol values from different families #1903

Open
fpagliughi opened this issue Dec 3, 2022 · 3 comments · May be fixed by #2068
Open

SockProtocol: No support for redundant protocol values from different families #1903

fpagliughi opened this issue Dec 3, 2022 · 3 comments · May be fixed by #2068

Comments

@fpagliughi
Copy link
Contributor

I wanted to send in a PR to add the CANbus socket protocols to SockProtocol, to include CanRaw (1) and CanBcm (2).

Like this:

    /// The Controller Area Network raw socket protocol
    /// ([ref](https://docs.kernel.org/networking/can.html#how-to-use-socketcan))
    #[cfg(any(target_os = "android", target_os = "linux"))]
    #[cfg_attr(docsrs, doc(cfg(all())))]
    CanRaw = libc::CAN_RAW,
    /// The Controller Area Network broadcast manager protocol
    /// ([ref](https://docs.kernel.org/networking/can.html#how-to-use-socketcan))
    #[cfg(any(target_os = "android", target_os = "linux"))]
    #[cfg_attr(docsrs, doc(cfg(all())))]
    CanBcm = libc::CAN_BCM,

The problem is that CAN_BCM = 2, which is already used by the discriminant, NetlinkUserSock = libc::NETLINK_USERSOCK.

Should SockProtocol be converted into an enum of enum's for each protocol family?
@asomers
Copy link
Member

asomers commented Dec 4, 2022

Annoying. But no, it shouldn't be an enum of enums. That would require either making separate socket functions for each AddressFamily, or using tagged enums, which aren't zero-cost. Instead, we should either use libc_bitflags!, or if this is the only violator we can add a const CanBcm = SockProtocol::NetlinkUserSock alias.

@fpagliughi
Copy link
Contributor Author

fpagliughi commented Dec 4, 2022 •
edited

Yeah, what I was imagining was a tagged outer enum, which, true, isn't zero cost, but seems to model the issue properly: Each protocol family has it's own enumeration of different protocols.

For that matter, though, we could probably infer both the family and protocol from the tagged outer enum! So one family/protocol parameter instead of two. That would even out the cost, but introduce some new issues.

It's just pretty funny that there wasn't an overlap with all the values that are already implemented.

But, anyway, I'm not looking to make this perfect here. I'm looking to solve an immediate couple of problems. The quick-fix to make CanBcm an external constant works well enough for me.

@fpagliughi
Copy link
Contributor Author

OK, PR #1912 is up for this, as well as #1914 which provides a CanAddr to bind the CAN socket.

@Dev380 Dev380 linked a pull request Jul 7, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Switch to SockProtocolInt Dev380/nix
2 participants
@asomers @fpagliughi