Re: [pbs-devel] [PATCH proxmox v4 3/4] proxmox-async: add connect_to_udp helper

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

A overly "sensitive" review due to getting a new public method, which is
always more work to change.


On 17.01.22 11:48, Dominik Csapak wrote:
> so that we do not have to always check the target ipaddr family manually

nit: above is slightly to long for our commit message style guide

> index 9a6d8a6..2dd49d4 100644
> --- a/proxmox-async/src/io/mod.rs
> +++ b/proxmox-async/src/io/mod.rs
> @@ -2,3 +2,6 @@
>  
>  mod async_channel_writer;
>  pub use async_channel_writer::AsyncChannelWriter;
> +
> +mod udp_connect;

nit: why not just udp? I mean, it's private so we can change any time without
breaking much, but feel still a bit to narrow/specialized - not really hard
feelings though.

> +pub use udp_connect::connect_to_udp;
> diff --git a/proxmox-async/src/io/udp_connect.rs b/proxmox-async/src/io/udp_connect.rs
> new file mode 100644
> index 0000000..878b150
> --- /dev/null
> +++ b/proxmox-async/src/io/udp_connect.rs
> @@ -0,0 +1,18 @@
> +use std::io;
> +use std::net::SocketAddr;
> +
> +use tokio::net::{ToSocketAddrs, UdpSocket};
> +
> +/// Helper to connect to UDP addresses without having to manually bind to the correct ip address
> +pub async fn connect_to_udp<A: ToSocketAddrs + std::fmt::Display>(

name is a bit weird, as one cannot connect to a UDP, maybe dropping the `to`,
i.e., `udp_connect`, would be already fine?

One alternative could be to make this module `pub mod udp` and name the helper
either just `connect` or `connect_to`, just throwing out the idea here, not
much preference.

> +    addr: A,
> +) -> io::Result<UdpSocket> {
> +    let socket = match tokio::net::lookup_host(&addr).await?.next() {> +        Some(SocketAddr::V4(_)) => UdpSocket::bind("0.0.0.0:0").await?,
> +        Some(SocketAddr::V6(_)) => UdpSocket::bind("[::]:0").await?,
> +        None => proxmox_sys::io_bail!("could not resolve address family {}", addr),

would it have some merit to use {:?} to loose the Display trait bound?
Probably not to relevant though.