Skip to content
This repository has been archived by the owner on Oct 26, 2022. It is now read-only.

Refactor base types #195

Merged
merged 11 commits into from
Nov 17, 2021
Merged

Conversation

stbuehler
Copy link
Contributor

Hi again.

#170 currently is broken because the workaround-audit-bug feature does bad things (it basically ignores the message length, which means the payload suddenly also contains the padding in my "test payload with length % 4 != 0" tests)

So I tried to get rid of that "feature" (also mentioned in #141 as "todo item"), and added Codec as type parameter to Connection.

As this is a major change anyway I tried to cleanup a few other things; those 4 commits should mostly be unrelated (I recommend reviewing them individually), so the subject of this PR just says "refactor" - but they all are API changes, so it makes somewhat sense to combine them (imho).

If this gets accepted I'm gonna basically have to redo #170 - so I'd like to get some feedback before I'll go back to #170 :)

@stbuehler
Copy link
Contributor Author

Hm. Seems I got some issues to fix:

  • tokio vs smol: seems the tests found a case where neither one is selected. Actually those features conflict too, which is not supported by cargo - probably should get fixed (in the same "bump" cycle?): it either needs another type parameter or internal boxing, and probably different entry points
  • The examples want to use Vec<u8> for recv... perhaps use bytes::buf::UninitSlice instead of &mut [MaybeUninit<u8>] - that would add bytes dependency to sys.

@stbuehler stbuehler force-pushed the refactor-base-types branch 2 times, most recently from e3b2b76 to e989c59 Compare November 14, 2021 14:20
@stbuehler stbuehler force-pushed the refactor-base-types branch 5 times, most recently from 058810b to 6c038a8 Compare November 14, 2021 21:04
`T` always was the same as `Self`.
Also do a major bump on depending crates.
… in implementations (apart from Debug)

- where clauses are almost never needed on type definitions unless you
  need them in the `Drop` implementation
- Debug trait might be useful if (debug) logging gets added
- Clone/PartialEq/Eq shouldn't be needed ever in the implementations
Copy link
Owner

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

Wow this is great!

The Codec thing has been bothering me from the start, and I didn't know how to improve the situation. I also love the AsyncSocket trait. I'm slightly bothered by making the netlink-sys API rely on bytes but the improvement is really worth it. Also, thank you for the soundness fix.

I'm going to let this open one day or two in case someone else wants to take a look, but I love it!

netlink-sys/src/smol.rs Outdated Show resolved Hide resolved
netlink-packet-core/src/traits.rs Show resolved Hide resolved
netlink-packet-audit/src/codec.rs Outdated Show resolved Hide resolved
netlink-packet-audit/src/codec.rs Show resolved Hide resolved
netlink-proto/src/codecs.rs Outdated Show resolved Hide resolved
netlink-proto/src/framed.rs Show resolved Hide resolved
netlink-sys/src/socket.rs Show resolved Hide resolved
1. get rid of "workaround-audit-bug" "feature"

2. no longer use tokio_util::codec::{Decoder, Encoder}

tokio_util::codec is "designed" for bytestreams (and building "frames"
of messages on top), but we need to deal with datagrams (which still
can contain multiples messages, just not one message across multiple
datagrams).

This make it a little less "combinable", but we actually don't want
people to reuse these codecs on bytestream (and writing an adapter
wouldn't be that hard anyway).

Also we can use a fixed error type, making dealing with it a little
bit easier.
Creating &mut [u8] (and &[u8]) for unitialized memory is undefined
behaviour even when not actually reading the data.

Use bytes::BufMut instead, and advance buffer in recv functions.
High level functions (without flags param) don't need to return length
of read, as it was used to advance the buffer - only low-level read
might return larger length than the buffer (PEEK + TRUNC flags).

This also bumps netlink-sys; the other crates already got bumped.
Preparation to make tokio and smol feature non-conflicting in
netlink-proto.
- This adds a new type paramater to connection for the socket being used.
- can drop tokio/smol selection from netlink-packet-audit
@little-dude
Copy link
Owner

Thanks @stbuehler

Do you want me to make new releases after this is merged, or do you want to rebase #170 and have releases once we have batch messages support?

@stbuehler
Copy link
Contributor Author

I'm slightly bothered by making the netlink-sys API rely on bytes but the improvement is really worth it.

Wanted to write a few words about that: &mut [MaybeUninit] can't be used without unsafe (on the "user" side - the implementation calling libc is unsafe anyway obviously). I think there are plans to come up with something better in std itself (for the Read trait), and perhaps then we can move to that.

Do you want me to make new releases after this is merged, or do you want to rebase #170 and have releases once we have batch messages support?

I don't need a release to continue working on #170 - I'd say that is up to you :)

@little-dude
Copy link
Owner

Wanted to write a few words about that: &mut [MaybeUninit] can't be used without unsafe (on the "user" side - the implementation calling libc is unsafe anyway obviously). I think there are plans to come up with something better in std itself (for the Read trait), and perhaps then we can move to that.

Sounds good. If you have links to specific RFCs/PRs or issues I'm interested!

For now, let's get this in! I'll make a release after you've rebased #170

Thanks for the quality work :)

@little-dude little-dude merged commit ee9bad4 into little-dude:master Nov 17, 2021
@stbuehler stbuehler deleted the refactor-base-types branch November 25, 2021 21:07
@little-dude little-dude mentioned this pull request Dec 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants