Re: [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition

[Aaron Lauterer <a.lauterer@proxmox.com>]

On  2024-03-29  12:43, Christoph Heiss wrote:
> Mostly just some comments regarding the struct (member) definitions, to
> make them (and their accompanying) checks a bit simpler.
> 
> On Thu, Mar 28, 2024 at 02:50:07PM +0100, Aaron Lauterer wrote:
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
> [..]
>> diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
>> new file mode 100644
>> index 0000000..96e5608
>> --- /dev/null
>> +++ b/proxmox-auto-installer/src/answer.rs
>> @@ -0,0 +1,256 @@
> [..]
>> +#[derive(Clone, Deserialize, Debug)]
>> +pub struct Global {
>> +    pub country: String,
>> +    pub fqdn: Fqdn,
>> +    pub keyboard: String,
>> +    pub mailto: String,
>> +    pub timezone: String,
>> +    pub password: String,
>> +    pub pre_command: Option<Vec<String>>,
>> +    pub post_command: Option<Vec<String>>,
>> +    pub reboot_on_error: Option<bool>,
> 
> How about using
> 
>      #[serde(default)]
>      pub reboot_on_error: bool,
> 
> here? Would make checks using this a bit simpler as well.

Thanks, I'll check it and also in the other instances.

> 
>> +}
>> +
>> +#[derive(Clone, Deserialize, Debug)]
>> +struct NetworkInAnswer {
>> +    pub use_dhcp: Option<bool>,
> 
> Same here as for `reboot_on_error`.
> 
>> +    pub cidr: Option<CidrAddress>,
>> +    pub dns: Option<IpAddr>,
>> +    pub gateway: Option<IpAddr>,
>> +    // use BTreeMap to have keys sorted
> 
> Since this comment appears multiple times in this, how about moving this
> into a module-level comment, explaining it there with a full sentence?

will do

> 
>> +    pub filter: Option<BTreeMap<String, String>>,
>> +}
>> +
> [..]
>> +
>> +#[derive(Clone, Deserialize, Debug)]
>> +pub struct DiskSetup {
>> +    pub filesystem: Filesystem,
>> +    pub disk_list: Option<Vec<String>>,
> 
> Could this be a
> 
>    #[serde(default)]
>    pub disk_list: Vec<String>,
> 
> as well? Both a missing & an empty list are invalid, right?

Yes, both would be invalid.

> 
>> +    // use BTreeMap to have keys sorted
>> +    pub filter: Option<BTreeMap<String, String>>,
>> +    pub filter_match: Option<FilterMatch>,
>> +    pub zfs: Option<ZfsOptions>,
>> +    pub lvm: Option<LvmOptions>,
>> +    pub btrfs: Option<BtrfsOptions>,
>> +}
>> +
> [..]
>> +
>> +impl TryFrom<DiskSetup> for Disks {
>> +    type Error = &'static str;
>> +
>> +    fn try_from(source: DiskSetup) -> Result<Self, Self::Error> {
>> +        if source.disk_list.is_none() && source.filter.is_none() {
>> +            return Err("Need either 'disk_list' or 'filter' set");
>> +        }
>> +        if source.disk_list.clone().is_some_and(|v| v.is_empty()) {
>                                ^^^^^^^^
> 
> nit: .as_ref() works here as well and would avoid a allocation.

thx

> 
> 
>> +            return Err("'disk_list' cannot be empty");
>> +        }
>> +        if source.disk_list.is_some() && source.filter.is_some() {
>> +            return Err("Cannot use both, 'disk_list' and 'filter'");
>> +        }
>> +
>> +        let disk_selection = match source.disk_list {
>> +            Some(disk_list) => DiskSelection::Selection(disk_list),
>> +            None => DiskSelection::Filter(source.filter.unwrap()),
>> +        };
>> +
>> +        // TODO: improve checks for foreign FS options. E.g. less verbose and handling new FS types
>> +        // automatically
>> +        let fs_options;
>> +        let fs = match source.filesystem {
> 
> This could be a
> 
>    let (fs, fs_options) = match source.filesystem {
> 
> ..
> 
>> +            Filesystem::Xfs => {
>> +                if source.zfs.is_some() || source.btrfs.is_some() {
>> +                    return Err("make sure only 'lvm' options are set");
>> +                }
>> +                fs_options = FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
>> +                FsType::Xfs
> 
> .. and then here instead
> 
>    (FsType::Xfs, FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default())))
> 
> as well for all the below cases, of course.

thx

> 
>> +            }
>> +            Filesystem::Ext4 => {
>> +                if source.zfs.is_some() || source.btrfs.is_some() {
>> +                    return Err("make sure only 'lvm' options are set");
>> +                }
>> +                fs_options = FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
>> +                FsType::Ext4
>> +            }
>> +            Filesystem::Zfs => {
>> +                if source.lvm.is_some() || source.btrfs.is_some() {
>> +                    return Err("make sure only 'zfs' options are set");
>> +                }
>> +                if source.zfs.is_none() || source.zfs.is_some_and(|v| v.raid.is_none()) {
>> +                    return Err("ZFS raid level 'zfs.raid' must be set");
>> +                }
>> +                fs_options = FsOptions::ZFS(source.zfs.unwrap());
>> +                FsType::Zfs(source.zfs.unwrap().raid.unwrap())
>> +            }
>> +            Filesystem::Btrfs => {
>> +                if source.zfs.is_some() || source.lvm.is_some() {
>> +                    return Err("make sure only 'btrfs' options are set");
>> +                }
>> +                if source.btrfs.is_none() || source.btrfs.is_some_and(|v| v.raid.is_none()) {
>> +                    return Err("BRFS raid level 'btrfs.raid' must be set");
>> +                }
>> +                fs_options = FsOptions::BRFS(source.btrfs.unwrap());
>> +                FsType::Btrfs(source.btrfs.unwrap().raid.unwrap())
> 
> Maybe make it a bit more succinctly like
> 
>      match source.btrfs {
> 	None => return Err(".."),
> 	Some(BtrfsOptions { raid: None, .. }) => return Err(".."),
> 	Some(opts) => (FsType::Btrfs(opts.raid.unwrap()), FsOptions::BRFS(opts)),
>      }
> 
>> +            }
>> +        };
>> +
>> +        let res = Disks {
>> +            fs_type: fs,
>> +            disk_selection,
>> +            filter_match: source.filter_match,
>> +            fs_options,
>> +        };
>> +        Ok(res)
>> +    }
>> +}
>> +
>> +#[derive(Clone, Debug)]
>> +pub enum FsOptions {
>> +    LVM(LvmOptions),
>> +    ZFS(ZfsOptions),
>> +    BRFS(BtrfsOptions),
>         ^^^^
> 
> The 'T' seems to got lost somewhere along the way :^)

oops... will fix that

> 
>> +}
>> +
>> +#[derive(Clone, Debug)]
>> +pub enum DiskSelection {
>> +    Selection(Vec<String>),
>> +    Filter(BTreeMap<String, String>),
>> +}
>> +#[derive(Clone, Deserialize, Debug, PartialEq)]
>> +#[serde(rename_all = "lowercase")]
>> +pub enum FilterMatch {
>> +    Any,
>> +    All,
>> +}
>> +
>> +#[derive(Clone, IntoEnumIterator, Deserialize, Serialize, Debug, PartialEq)]
> 		   ^^^^^^^^^^^^^^^^
> 
> Is this trait actually used somewhere or just some dead leftover?
> Not familiar with the crate, but removing this still compiles everything
> (aka. `make deb`) fine.

Needed for the CLI interfaces for CLAP IIRC.

> 
> If it's really just a leftover, the whole crate dependency could be
> dropped.
> 
>> +#[serde(rename_all = "lowercase")]
>> +pub enum Filesystem {
>> +    Ext4,
>> +    Xfs,
>> +    Zfs,
>> +    Btrfs,
>> +}
>> +
>> +#[derive(Clone, Copy, Default, Deserialize, Debug)]
>> +pub struct ZfsOptions {
>> +    pub raid: Option<ZfsRaidLevel>,
>> +    pub ashift: Option<usize>,
>> +    pub arc_max: Option<usize>,
>> +    pub checksum: Option<ZfsChecksumOption>,
>> +    pub compress: Option<ZfsCompressOption>,
>> +    pub copies: Option<usize>,
>> +    pub hdsize: Option<f64>,
>> +}
>> +
>> +impl ZfsOptions {
>> +    pub fn new() -> ZfsOptions {
>> +        ZfsOptions::default()
>> +    }
>> +}
> 
> Again, needed? Doesn't seem to be used anywhere.

It's been a while since I touched that code. But IIRC it wouldn't 
deserialize otherwise. I'll check it again.
Also for the other instances like this.