[pve-devel] [PATCH proxmox v4 1/5] network-types: initial commit
Gabriel Goller
g.goller at proxmox.com
Fri Jul 4 14:19:45 CEST 2025
>> [snip]
>> # workspace dependencies
>> -proxmox-access-control = { version = "0.2.5", path = "proxmox-access-control" }
>> -proxmox-acme = { version = "1.0.0", path = "proxmox-acme", default-features = false }
>> -proxmox-api-macro = { version = "1.4.0", path = "proxmox-api-macro" }
>> -proxmox-apt-api-types = { version = "2.0.0", path = "proxmox-apt-api-types" }
>> -proxmox-auth-api = { version = "1.0.0", path = "proxmox-auth-api" }
>> -proxmox-async = { version = "0.5.0", path = "proxmox-async" }
>> -proxmox-base64 = { version = "1.0.0", path = "proxmox-base64" }
>> -proxmox-compression = { version = "1.0.0", path = "proxmox-compression" }
>> -proxmox-daemon = { version = "1.0.0", path = "proxmox-daemon" }
>> -proxmox-http = { version = "1.0.0", path = "proxmox-http" }
>> -proxmox-http-error = { version = "1.0.0", path = "proxmox-http-error" }
>> -proxmox-human-byte = { version = "1.0.0", path = "proxmox-human-byte" }
>> -proxmox-io = { version = "1.2.0", path = "proxmox-io" }
>> -proxmox-lang = { version = "1.5", path = "proxmox-lang" }
>> -proxmox-log = { version = "1.0.0", path = "proxmox-log" }
>> -proxmox-login = { version = "1.0.0", path = "proxmox-login" }
>> -proxmox-product-config = { version = "1.0.0", path = "proxmox-product-config" }
>> -proxmox-config-digest = { version = "1.0.0", path = "proxmox-config-digest" }
>> -proxmox-rest-server = { version = "1.0.0", path = "proxmox-rest-server" }
>> -proxmox-router = { version = "3.2.2", path = "proxmox-router" }
>> -proxmox-schema = { version = "4.1.0", path = "proxmox-schema" }
>> -proxmox-section-config = { version = "3.1.0", path = "proxmox-section-config" }
>> -proxmox-sendmail = { version = "1.0.0", path = "proxmox-sendmail" }
>> -proxmox-serde = { version = "1.0.0", path = "proxmox-serde", features = [ "serde_json" ] }
>> -proxmox-shared-memory = { version = "1.0.0", path = "proxmox-shared-memory" }
>> +proxmox-acme = { version = "0.5.3", path = "proxmox-acme", default-features = false }
>> +proxmox-api-macro = { version = "1.3.2", path = "proxmox-api-macro" }
>> +proxmox-apt-api-types = { version = "1.0.2", path = "proxmox-apt-api-types" }
>> +proxmox-auth-api = { version = "0.4.0", path = "proxmox-auth-api" }
>> +proxmox-async = { version = "0.4.1", path = "proxmox-async" }
>> +proxmox-compression = { version = "0.2.4", path = "proxmox-compression" }
>> +proxmox-daemon = { version = "0.1.0", path = "proxmox-daemon" }
>> +proxmox-http = { version = "0.9.5", path = "proxmox-http" }
>> +proxmox-http-error = { version = "0.1.0", path = "proxmox-http-error" }
>> +proxmox-human-byte = { version = "0.1.0", path = "proxmox-human-byte" }
>> +proxmox-io = { version = "1.1.0", path = "proxmox-io" }
>> +proxmox-lang = { version = "1.3", path = "proxmox-lang" }
>> +proxmox-log= { version = "0.2.9", path = "proxmox-log" }
>> +proxmox-login = { version = "0.2.0", path = "proxmox-login" }
>> +proxmox-network-types = { version = "0.1.0", path = "proxmox-network-types" }
>> +proxmox-product-config = { version = "0.2.0", path = "proxmox-product-config" }
>> +proxmox-config-digest = { version = "0.1.0", path = "proxmox-config-digest" }
>> +proxmox-rest-server = { version = "0.8.8", path = "proxmox-rest-server" }
>> +proxmox-router = { version = "3.1.1", path = "proxmox-router" }
>> +proxmox-schema = { version = "4.0.0", path = "proxmox-schema" }
>> +proxmox-section-config = { version = "3.0.0", path = "proxmox-section-config" }
>> +proxmox-sendmail = { version = "0.1.0", path = "proxmox-sendmail" }
>> +proxmox-serde = { version = "0.1.1", path = "proxmox-serde", features = [ "serde_json" ] }
>> +proxmox-shared-memory = { version = "0.3.0", path = "proxmox-shared-memory" }
>> proxmox-sortable-macro = { version = "0.1.3", path = "proxmox-sortable-macro" }
>
>^ Reverts to bookworm deps - you sure the patches are otherwise based on
>trixie?
Missed this hunk, my bad. The rest should be rebased on trixie
correctly.
>> diff --git a/proxmox-network-types/src/ip_address.rs b/proxmox-network-types/src/ip_address.rs
>> new file mode 100644
>> index 000000000000..355547b17ae0
>> --- /dev/null
>> +++ b/proxmox-network-types/src/ip_address.rs
>> @@ -0,0 +1,1410 @@
>> +//! Provides helpers to deal with IP addresses / CIDRs
>> +
>> +use std::net::{AddrParseError, IpAddr, Ipv4Addr, Ipv6Addr};
>> +
>> +use serde_with::{DeserializeFromStr, SerializeDisplay};
>> +use thiserror::Error;
>> +
>> +/// The family (v4 or v6) of an IP address or CIDR prefix
>> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
>> +pub enum Family {
>> + V4,
>> + V6,
>> +}
>> +
>> +impl Family {
>> + pub fn is_ipv4(&self) -> bool {
>> + *self == Self::V4
>> + }
>
>nit: ↑↓ those could take self copied since it's Family is Copy.
Done.
>> +
>> + pub fn is_ipv6(&self) -> bool {
>> + *self == Self::V6
>> + }
>> +}
>> +
>> +impl std::fmt::Display for Family {
>> + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
>> + match self {
>> + Family::V4 => f.write_str("Ipv4"),
>> + Family::V6 => f.write_str("Ipv6"),
>
>For a Display implementation it IMO makes no sense to mix
>capital/lowercase in "IP". If we need to keep this for ser/de purposes,
>add a comment please. Otherwise change it to IPv4/IPv6.
Agree.
>> + }
>> + }
>> +}
>> +
>> +#[derive(Error, Debug)]
>> +pub enum CidrError {
>> + #[error("invalid netmask")]
>> + InvalidNetmask,
>> + #[error("invalid IP address")]
>> + InvalidAddress(#[from] AddrParseError),
>> +}
>> +
>> +/// Represents either an [`Ipv4Cidr`] or [`Ipv6Cidr`] CIDR prefix
>> +#[derive(
>> + Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, SerializeDisplay, DeserializeFromStr,
>> +)]
>> +pub enum Cidr {
>> + Ipv4(Ipv4Cidr),
>> + Ipv6(Ipv6Cidr),
>> +}
>> +
>> +impl Cidr {
>> + pub fn new_v4(addr: impl Into<Ipv4Addr>, mask: u8) -> Result<Self, CidrError> {
>> + Ok(Cidr::Ipv4(Ipv4Cidr::new(addr, mask)?))
>> + }
>> +
>> + pub fn new_v6(addr: impl Into<Ipv6Addr>, mask: u8) -> Result<Self, CidrError> {
>> + Ok(Cidr::Ipv6(Ipv6Cidr::new(addr, mask)?))
>> + }
>> +
>> + /// which [`Family`] this CIDR belongs to
>> + pub const fn family(&self) -> Family {
>> + match self {
>> + Cidr::Ipv4(_) => Family::V4,
>> + Cidr::Ipv6(_) => Family::V6,
>> + }
>> + }
>> +
>> + pub fn is_ipv4(&self) -> bool {
>> + matches!(self, Cidr::Ipv4(_))
>> + }
>> +
>> + pub fn is_ipv6(&self) -> bool {
>> + matches!(self, Cidr::Ipv6(_))
>> + }
>> +
>> + /// Whether a given IP address is contained in this [`Cidr`]
>> + ///
>> + /// This only works if both [`IpAddr`] are in the same family, otherwise the function returns
>> + /// false.
>> + pub fn contains_address(&self, ip: &IpAddr) -> bool {
>> + match (self, ip) {
>> + (Cidr::Ipv4(cidr), IpAddr::V4(ip)) => cidr.contains_address(ip),
>> + (Cidr::Ipv6(cidr), IpAddr::V6(ip)) => cidr.contains_address(ip),
>> + _ => false,
>> + }
>> + }
>> +}
>> +
>> +impl std::fmt::Display for Cidr {
>> + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
>> + match self {
>> + Self::Ipv4(ip) => f.write_str(ip.to_string().as_str()),
>> + Self::Ipv6(ip) => f.write_str(ip.to_string().as_str()),
>
>No need to allocate a new string if you just forward to the inner value,
>either use `write!(f, ip)` (more convenient), or explicitly forward via
>`Display::fmt(ip, f)`:
>
> Self::Ipv4(ip) => fmt::Display::fmt(ip, f),
> Self::Ipv6(ip) => fmt::Display::fmt(ip, f),
>
Would have been `write!(f, "{}", ip)`, so I went with Display::fmt :)
>> + }
>> + }
>> +}
>> +
>> +impl std::str::FromStr for Cidr {
>> + type Err = CidrError;
>> +
>> + fn from_str(s: &str) -> Result<Self, Self::Err> {
>> + if let Ok(ip) = s.parse::<Ipv4Cidr>() {
>> + return Ok(Cidr::Ipv4(ip));
>> + }
>> +
>> + Ok(Cidr::Ipv6(s.parse()?))
>> + }
>> +}
>> +
>> +impl From<Ipv4Cidr> for Cidr {
>> + fn from(cidr: Ipv4Cidr) -> Self {
>> + Cidr::Ipv4(cidr)
>> + }
>> +}
>> +
>> +impl From<Ipv6Cidr> for Cidr {
>> + fn from(cidr: Ipv6Cidr) -> Self {
>> + Cidr::Ipv6(cidr)
>> + }
>> +}
>> +
>> +impl From<IpAddr> for Cidr {
>> + fn from(value: IpAddr) -> Self {
>> + match value {
>> + IpAddr::V4(addr) => Ipv4Cidr::from(addr).into(),
>> + IpAddr::V6(addr) => Ipv6Cidr::from(addr).into(),
>> + }
>> + }
>> +}
>> +
>> +const IPV4_LENGTH: u8 = 32;
>> +
>> +/// An IPv4 CIDR (e.g. 192.0.2.0/24)
>> +#[derive(
>> + SerializeDisplay, DeserializeFromStr, Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash,
>
>nit: would prefer the standard stuff first
Agree
>> [snip]
>> +impl std::str::FromStr for Ipv4Cidr {
>> + type Err = CidrError;
>> +
>> + fn from_str(s: &str) -> Result<Self, Self::Err> {
>> + Ok(match s.find('/') {
>> + None => Self {
>> + addr: s.parse()?,
>> + mask: 32,
>> + },
>> + Some(pos) => {
>> + let mask: u8 = s[(pos + 1)..]
>> + .parse()
>> + .map_err(|_| CidrError::InvalidNetmask)?;
>> +
>> + Self::new(s[..pos].parse::<Ipv4Addr>()?, mask)?
>> + }
>> + })
>
>^ since 1.52 this could be more readable with `.split_once('/')`, then
>we don't need to slice manually with `..pos`/`(pos + 1)..`.
Fixed this.
>> + }
>> +}
>> +
>> +impl std::fmt::Display for Ipv4Cidr {
>> + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
>> + write!(f, "{}/{}", &self.addr, self.mask)
>
>^ unnecessary &
Agree.
>> [snip]
>> +#[cfg(test)]
>> +mod tests {
>> + use super::*;
>> + use std::net::{Ipv4Addr, Ipv6Addr};
>
>^ std import should be grouped befure the `super::*` one.
cargo fmt puts super over std :(
Thanks for the review!
More information about the pve-devel
mailing list