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

[Dominik Csapak <d.csapak@proxmox.com>]

On 2/1/22 13:02, Thomas Lamprecht wrote:
> 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.

yes, udp::connect() does look better & cleaner

> 
>> +    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.
> 

then we'd need the Debug trait though, so no real gain?