Re: [pve-devel] [PATCH proxmox-perl-rs 04/11] fabrics: add CRUD and generate fabrics methods

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

>> [snip]
>> +    impl PerlSectionConfig<OpenFabricSectionConfig> {
>> +        pub fn add_fabric(&self, new_config: AddFabric) -> Result<(), anyhow::Error> {
>> +            let fabricid = FabricId::from(new_config.name).to_string();
>
>Could we simplify this method and the ones below by just using the
>concrete types (here FabricId) inside the argument structs (AddFabric)?
>There's potential for quite a few here afaict, also with the
>Option<u16>'s. Would save us a lot of conversion / validation logic if
>we just did it at deserialization.

Yep, that would work. We just need to change the serde deserialize
override to be generic.

>I pointed out some instances below.
>
>I guess the error messages would be a bit worse then?

Nope, they are quite the same. We can just wrap them in serde custom
errors.

>> +            let new_fabric = OpenFabricSectionConfig::Fabric(FabricSection {
>> +                hello_interval: new_config
>> +                    .hello_interval
>> +                    .map(|x| x.try_into())
>> +                    .transpose()?,
>> +            });
>> +            let mut config = self.section_config.lock().unwrap();
>> +            if config.sections.contains_key(&fabricid) {
>> +                anyhow::bail!("fabric already exists");
>> +            }
>> +            config.sections.insert(fabricid, new_fabric);
>
>try_insert instead of contains_key + insert?

still deprecated :)

>> [snip]
>> +            let mut config = self.section_config.lock().unwrap();
>> +            if !config.sections.contains_key(&nodeid) {
>> +                anyhow::bail!("node not found");
>> +            }
>> +            config.sections.entry(nodeid).and_modify(|n| {
>> +                if let OpenFabricSectionConfig::Node(n) = n {
>> +                    n.net = net;
>> +                    n.interface = interfaces;
>> +                }
>> +            });
>
>wouldn't get_mut be easier here? also would save the extra contains_key

Agree, also changed everywhere else.

Thanks!


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