Re: [pve-devel] [PATCH proxmox-perl-rs v2 2/4] fabrics: add function to get status of fabric

[Gabriel Goller <g.goller@proxmox.com>]

On 26.08.2025 09:55, Wolfgang Bumiller wrote:
>On Mon, Aug 25, 2025 at 05:33:49PM +0200, Gabriel Goller wrote:
>> On 25.08.2025 16:37, Wolfgang Bumiller wrote:
>> > On Mon, Aug 25, 2025 at 01:39:48PM +0200, Gabriel Goller wrote:
>> > > On 25.08.2025 10:11, Wolfgang Bumiller wrote:
>> > > > > diff --git a/pve-rs/src/bindings/sdn/fabrics.rs b/pve-rs/src/bindings/sdn/fabrics.rs
>> > > > > index 1dc8bf4320e6..3f70d421e582 100644
>> > > > > --- a/pve-rs/src/bindings/sdn/fabrics.rs
>> > > > > +++ b/pve-rs/src/bindings/sdn/fabrics.rs
>> > > > > @@ -9,8 +9,10 @@ pub mod pve_rs_sdn_fabrics {
>> > > > >      use std::fmt::Write;
>> > > > >      use std::net::IpAddr;
>> > > > >      use std::ops::Deref;
>> > > > > +    use std::process::Command;
>> > > > >      use std::sync::Mutex;
>> > > > >
>> > > > > +    use anyhow::Context;
>> > > > >      use anyhow::Error;
>> > > > >      use openssl::hash::{MessageDigest, hash};
>> > > > >      use serde::{Deserialize, Serialize};
>> > > > > @@ -578,4 +580,196 @@ pub mod pve_rs_sdn_fabrics {
>> > > > >
>> > > > >          Ok(interfaces)
>> > > > >      }
>> > > > > +
>> > > > > +    /// This module contains status-related structs that represent Routes and Neighbors for all
>> > > > > +    /// protocols
>> > > > > +    pub mod status {
>> > > >
>> > > > ^ This seems to be a helper module which does not contain any
>> > > > perlmod/perl specifics.
>> > > > I'd argue it's time to start a `crate::sdn` module outside of the
>> > > > `bindings` submodule for this.
>> > > >
>> > > > The `bindings` module should become rather more lean in the future and
>> > > > focus mostly on the perl/rust interaction.
>> > >
>> > > Umm do I understand you correctly that you want to have something like
>> > > this:
>> > >
>> > >     src/
>> > >     ├─ bindings/
>> > >     │  ├─ sdn/
>> > >     │  │  ├─ fabrics.rs
>> > >     ├─ sdn/
>> > >     │  ├─ status.rs
>> > >
>> > > ?
>> >
>> > Yes. The bindings should just be the perl interface and its point is to
>> > provide documentation via rustdoc, and the rustdocs should tell you how
>> > to use it *from perl*.
>> >
>> > The rest would be additional code we need to provide the perl interface
>> > for the external crates.
>> >
>> > > IMO we could move all the status stuff out to
>> > > crate::bindings::sdn::status. But I don't know about separating all the
>> > > types, conversion methods and actual perl methods -- I'd rather keep all
>> > > the perl-facing stuff in the same file.
>> >
>> > You don't need to separate everything. My point was that it does *not*
>> > contain perl *binding* specifics. As for being *perl* specific, I mean,
>> > the entire `pve-rs` crate *is* perl specific right now...
>>
>> True :)
>> Should we maybe have a 'types' module in the middle, so that we have:
>>
>> src/bindings/sdn/fabrics.rs
>> src/types/sdn/fabrics.rs
>
>Not really a fan of this. I'd rather have the toplevel modules by
>feature.

ACK

>This *may* make sense if/when we start putting API types there and
>serialize the schema out into perl, which is something we should
>take into serious consideration soon-ish. It may be the complement or
>even improve the situation with the pve-api-types crate. (While *at
>first* it will definitely be a nuisance with where to find type
>definitions and getting them updated... but in the long run? Anyway...
>this is out of scope here :) )

Agree.


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