Re: [pve-devel] [PATCH proxmox-ve-rs v4 03/22] sdn-types: initial commit

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Jul 04, 2025 at 04:40:47PM +0200, Gabriel Goller wrote:
> > > [snip]
> > > diff --git a/proxmox-sdn-types/src/net.rs b/proxmox-sdn-types/src/net.rs
> > > new file mode 100644
> > > index 000000000000..78a47983f0c7
> > > --- /dev/null
> > > +++ b/proxmox-sdn-types/src/net.rs
> > > @@ -0,0 +1,329 @@
> > > +use std::{
> > > +    fmt::Display,
> > > +    net::{IpAddr, Ipv4Addr, Ipv6Addr},
> > > +};
> > > +
> > > +use anyhow::{bail, Error};
> > > +use const_format::concatcp;
> > > +use serde::{Deserialize, Serialize};
> > > +
> > > +use proxmox_schema::{api, api_string_type, const_regex, ApiStringFormat, UpdaterType};
> > > +
> > > +const NET_AFI_REGEX_STR: &str = r"(?:[a-fA-F0-9]{2})";
> > 
> > Would it make sense to represent the `NetAFI` type as an `u8`?
> > Could then be `Copy` and wouldn't need to allocate a string.
> 
> Stefan initially suggested that we could display the whole NET as [u8, n],
> but we then discarded it as there would be some components which are not
> in a power of 2 size which would make the handling tricky.
> 
> I am kinda wary of representing some parts of the NET as u8 and some as
> String, I think that only makes it more complex and isn't really worth
> the hassle.
> 
> If you think this is important though, I'll give it a shot...

Not important. It just seems like as something that's effectively a byte
it could be nice to have a `Copy` type for it.
Not sure why it would really be a hassle, given that it is its own type
and the internal representation shouldn't matter to the rest of the
code.
But no, it's not important, and can be changed later anyway.

> 
> > > +const NET_AREA_REGEX_STR: &str = r"(?:[a-fA-F0-9]{4})";
> > > +const NET_SYSTEM_ID_REGEX_STR: &str = r"(?:[a-fA-F0-9]{4})\.(?:[a-fA-F0-9]{4})\.(?:[a-fA-F0-9]{4})";
> > > +const NET_SELECTOR_REGEX_STR: &str = r"(?:[a-fA-F0-9]{2})";
> > 
> > ^ Same for the selector.
> > 
> > > +
> > > +const_regex! {
> > > +    NET_AFI_REGEX = concatcp!(r"^", NET_AFI_REGEX_STR, r"$");
> > > +    NET_AREA_REGEX = concatcp!(r"^", NET_AREA_REGEX_STR, r"$");
> > > +    NET_SYSTEM_ID_REGEX = concatcp!(r"^", NET_SYSTEM_ID_REGEX_STR, r"$");
> > > +    NET_SELECTOR_REGEX = concatcp!(r"^", NET_SELECTOR_REGEX_STR, r"$");
> > 
> > ^ Why don't we anchor the consts already instead of concating that here?
> > Or - if they are only used this once we could just inline them here?
> 
> Yeah, we could probably inline them here, I think we just copied this
> from PBS (where we always do this).

Right. I don't think it makes sense this way, mostly because the
constants aren't `pub` anyway, so if at some point one would need access
to them, the crate would need a bump to add the `pub` *anyway*, so it
could then be turned back into a const. (And for crate-internal access
it's also just a tiny commit before using it...)

> 
> > > +}
> > > +
> > > +const NET_AFI_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&NET_AFI_REGEX);
> > > +const NET_AREA_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&NET_AREA_REGEX);
> > > +const NET_SYSTEM_ID_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&NET_SYSTEM_ID_REGEX);
> > > +const NET_SELECTOR_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&NET_SELECTOR_REGEX);
> > > +
> > > [snip]


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel