many: port from varlink & varlink_parser to zlink#7
Conversation
|
Hey, thanks a bunch for your work! I think using claude here is indeed fine so no problem here. The commit does contain Cargo.lock which is already part of #3 and #6 - would you mind removing it and repush? fwiw, I have no strong opinion on varlink/rust vs zlink, I have some work ready (like https://github.com/systemd/varlink-http-bridge/compare/main...mvo5:varlink-http-bridge:varlink-more-no-sse?expand=1 that uses the "more" call but I assume that will be easy to port(?). |
👍
Oops. I'll do that shortly.
Fair enough. This is why I mentioned the benefits. :) I have some work ready (like https://github.com/systemd/varlink-http-bridge/compare/main...mvo5:varlink-http-bridge:varlink-more-no-sse?expand=1 that uses the "more" call but I assume that will be easy to port(?). zlink supports |
mvo5
left a comment
There was a problem hiding this comment.
Thanks for your PR and for the update without the Cargo.toml. Lots to like, I have a few questions/suggestions inline though (I did enjoy your ASG talk about zlink btw)
Replace `varlink` and `varlink_parser` crates with `zlink`, the modern async-first replacement. Key improvements: 1. IDL parsing is ~3.8× faster (1.7 µs vs 6.4 µs). 2. Typed Proxy trait — `get_info()` and `get_interface_description()` as first-class methods on Connection, replacing manual AsyncMethodCall construction with string-based method names. 3. No Arc wrapping — zlink's Connection uses `&mut self` directly, removing the need for Arc<AsyncConnection>. 4. Better error separation — transport errors (SocketRead, Io, etc.) vs application errors (VarlinkService) are cleanly separated, enabling more precise HTTP status mapping: - PermissionDenied → 403 (new) - InterfaceNotFound → 404 (new) - ExpectedMore → 400 (new) 5. Socket paths no longer need the `unix:` prefix — zlink's `unix::connect()` takes plain filesystem paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thanks! 🙂 I believe I have addressed all your comments/suggestions now, except for 2, where you get to decide what to do. Also, I took the liberty of making things slightly more efficient using references. Do let me know if you don't want those changes and I can revert them. |
| parameters: Some(&call_args), | ||
| }); | ||
| connection | ||
| .call_method::<_, DynReply<'_>, DynReplyError<'_>>(&call, vec![]) |
There was a problem hiding this comment.
This might be a silly question/request - I ask it anyway :) I wonder if zlink could have something like a call_method_raw(&method, &call_args) that just takes serde_json::Value directly? AFAICT it would make this piece (that is a bit hard to read for me) a lot nicer? I guess the use-cases are only when doing dynamic varlink calls which is a bit of a special use-case but here is a consumer for it :)
There was a problem hiding this comment.
Firstly, you're not going to be the only user of a dynamic API. :) However, this is already quite dynamic and you can use a Value but I switched to a more concrete type on purpose here. The let call = ... lines can actually be removed here by replacing &call on this line with &call.into() because of impl<M> From<M> for Call<M>. I'll add this to my todo for the follow-up PR.
There was a problem hiding this comment.
Ok, maybe its just me having not yet done enough go, the .call_method::<_, DynReply<'_>, DynReplyError<'_>>(&call, vec![]) looks a bit daunting and in https://github.com/systemd/varlink-http-bridge/compare/main...mvo5:port-zlink-pr-1?expand=1#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR453 would just become let reply = connection.call_dynamic(&method, call_args).await??; which feels (to me) nicer - but again, might just be my ignorance speaking.
There was a problem hiding this comment.
I think it should be now more readable in #12. However, I realized that we still do need to construct the DynMethod type. Perhaps it's possible to do this better but I'll need to learn a lot more of axum API then I currently do, to be able to tell. 😢
P.S. FWIW this looks quite readable to me but maybe it's because I've seen much worse. :)
There was a problem hiding this comment.
oops, GH only showed me your last comment after I had created the PR and commented here. 😢 let reply = connection.call_dynamic(&method, call_args).await??; could be good but then you've to Ok wrap it again, which is why I've always preferred .map_err(Into::into) but it's also because over the years I've learnt to like call chaining (I forgot the correct word for this pattern 😆).
Anyway, check out #12 and if you think it can be even simpler/more readable, feel free to change the code as you see fit. I've no strong opinions here. :)
mvo5
left a comment
There was a problem hiding this comment.
Thanks! I like it, especially the better error handling seems like a really nice feature!
Replace
varlinkandvarlink_parsercrates withzlink, the modern async-first replacement.Key improvements:
IDL parsing is ~3.8× faster (1.7 µs vs 6.4 µs, see benchmarks).
Typed Proxy trait —
get_info()andget_interface_description()as first-class methods on Connection, replacing manual AsyncMethodCall construction with string-based method names.No Arc wrapping — zlink's Connection uses
&mut selfdirectly, removing the need for Arc.Better error separation — transport errors (SocketRead, Io, etc.) vs application errors (VarlinkService) are cleanly separated, enabling more precise HTTP status mapping:
unix:prefix — zlink'sunix::connect()takes plain filesystem paths.Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Notes: