gluxon / wireguard-uapi-rs

MIT License
32 stars 10 forks source link

update neli to 0.6.3 #26

Closed phi-gamma closed 1 year ago

phi-gamma commented 2 years ago
API changes:

- impl_var!() removed in 0.6.0.
- Trait Nl removed in 0.6.0.
- Stream{Read,Write}Buffer have been obsoleted.
- There's an enum IflaInfo for IFLA_INFO_* defines.
- NlPayload type for Genlmsghdr payload.
- Track NlSocketHandle API changes (::connect() over ::open()/::bind()).
- Simplified header deserialization via FromBytesWithInput trait.
- The asize() member function disappeared.
phi-gamma commented 2 years ago

Hi, I need wireguard-uapi to work against a more up-to-date neli so I had a stab at updating the dependency. Please tell me if you’re interested, I’ve been using it for about a week now without running into issues.

gluxon commented 2 years ago

Hi, I need wireguard-uapi to work against a more up-to-date neli so I had a stab at updating the dependency.

Hey @phi-gamma, this seems reasonable. Thanks for the work here! 🙂

As a heads up, @ernestask did take a stab at incrementally going to 0.5.3 https://github.com/gluxon/wireguard-uapi-rs/pull/21, but we put it on hold since I had longer-term plans of using a more generic Netlink RPC. We can get this going again since multiple developers are looking for this now though, and I don't want to block on long-term plans that may not come to fruition.

@ernestask Do you have thoughts on the changes here? I'm happy to re-open and merge your changes first to get us on 0.5.3 incrementally since your PR was first.

ernestask commented 2 years ago

Well, it would be easier to review from 0.5 and then rebase for sure.

ernestask commented 2 years ago

Though this adds unnecessary workload on @phi-gamma, so I could just review this as-is.

phi-gamma commented 2 years ago

Sorry, I hadn’t seen that earlier PR so no idea how much they differ. I could look into rebasing on top of it sure but it may take some time until I get around to it as I’ve got a four weeks vacation coming up.

gluxon commented 2 years ago

Though this adds unnecessary workload on @phi-gamma, so I could just review this as-is.

I'd be okay with both options. Happy to go with your preference @ernestask.

If we decide to merge https://github.com/gluxon/wireguard-uapi-rs/pull/21 first, I can help rebase the changes here on top of the 0.5.3 to reduce the unnecessary workload on @phi-gamma. Would that be appreciated @phi-gamma, or would you prefer to do that yourself if we go this route?

phi-gamma commented 2 years ago

I’m indifferent as well, just to return the ball to @ernestask. ;) Though rebasing via 0.5 might make the git history more logical.

I’ll try and take on the rebase tomorrow morning, let’s see how far I get.

phi-gamma commented 2 years ago

Rebased.

I tried to preserve as much of @ernestask’s work as I could. Those TryFrom impls in particular are nifty. :)

phi-gamma commented 1 year ago

Just bumped the neli dep to 0.6.2. There seems to be some issue with the CI which runs the enodev test that isn’t even on this branch. From the diffs I consider it very unlikely that this is breakage due to changes in neli.

I can reproduce this here with both neli 0.6.1 and 0.6.2, rebased on main. Same for neli 0.5.3.

I now rebased the branch on main and adapted the error message. After the library update we’re getting that ENODEV as expected, however from the comment it looks like besides that you’re also interested in a differently worded error message. Could you maybe comment on that?

gluxon commented 1 year ago

I now rebased the branch on main and adapted the error message. After the library update we’re getting that ENODEV as expected, however from the comment it looks like besides that you’re also interested in a differently worded error message. Could you maybe comment on that?

Your update looks right. We can followup after this PR merges with a better wording. 🙂

To unblock the base PR, I ended up cherry-picking your fix over to https://github.com/gluxon/wireguard-uapi-rs/pull/21/commits/29c04ba8f3108e21b3b66b6cf0fa9e933d7debd0. Thanks!

gluxon commented 1 year ago

A few conflicts appeared after https://github.com/gluxon/wireguard-uapi-rs/pull/21 merged. @phi-gamma Would you appreciate my help resolving those conflicts, or would you prefer to do that yourself?

phi-gamma commented 1 year ago

Branch adapted, please have a look now!

phi-gamma commented 1 year ago

@@ -132,7 +123,7 @@ impl WgSocket { pub fn set_device(&mut self, device: set::Device) -> Result<(), SetDeviceError> { for nl_message in create_set_device_messages(device, self.family_id)? { self.sock.send(nl_message)?;

  • self.sock.recv::<Nlmsg, Genlmsghdr<CtrlCmd, CtrlAttr>>()?;
  • self.sock.recv()?;

Do you recall why the turbofish operator here was removed?

Not anymore, no. Trying to restore the old line, I see rustc complain about a missing error conversion trait so maybe it was a the “lazy fix” to that? It’s been a while so I can’t say for certain what the motivation was.

gluxon commented 1 year ago

@phi-gamma Thanks again for these changes! I was admittedly a bit nervous about this upgrade, but your changes look good. Appreciate the patience.

gluxon commented 1 year ago

Released v3.0.0-rc1. https://github.com/gluxon/wireguard-uapi-rs/releases/tag/v3.0.0-rc1

@phi-gamma @ernestask Give a heads up if you see any issues. Otherwise we can release v3.0.0 in a few weeks.

phi-gamma commented 1 year ago

Merged #26 into main.

Great news, thanks a lot!

thomasjfox commented 1 year ago

Some feedback on v3.0.0-rc1:

I've taken the previous version 2.0.5, applied the neli 0.5.3 + 0.6.1 patches and diffed the output compared to v3.0.0-rc1. The only noteworthy change was:

Commit https://github.com/gluxon/wireguard-uapi-rs/commit/f595e5d314d5bc1d6f28ce7696c215089d14cfc9 "Iterate over Ifinfomsg attributes once when listing names"

Next I've taken v3.0.0-rc1 and ran integration tests for a custom Wireguard configuration daemon. All tests passed and they do stuff like change allowed IPs or keepalive intervals at runtime.

The new code has been in production for over a week and hums away.

Thanks a lot @gluxon for merging the neli 0.6.x upgrade.

gluxon commented 1 year ago

3.0.0 has been released with the changes here! https://github.com/gluxon/wireguard-uapi-rs/releases/tag/v3.0.0

@thomasjfox Thank you so much for those testing notes. It made releasing the large neli updates over the last few months less scary.