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

[Christoph Heiss <c.heiss@proxmox.com>]

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.

> +}
> +
> +#[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?

> +    pub filter: Option<BTreeMap<String, String>>,
> +}
> +
[..]
> +
> +#[derive(Clone, Debug)]
> +pub enum NetworkSettings {
> +    Dhcp(bool),

`Dhcp` as variant without the `bool` should work too. At least nothing
seems to exlicitly match on this variant from a quick grep.

> +    Manual(NetworkManual),
> +}
> +
> +#[derive(Clone, Debug)]
> +pub struct NetworkManual {
> +    pub cidr: CidrAddress,
> +    pub dns: IpAddr,
> +    pub gateway: IpAddr,
> +    // use BTreeMap to have keys sorted
> +    pub filter: 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?

> +    // 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.


> +            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.

> +            }
> +            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 :^)

> +}
> +
> +#[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.

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.

> +
> +#[derive(Clone, Copy, Default, Deserialize, Serialize, Debug)]
> +pub struct LvmOptions {
> +    pub hdsize: Option<f64>,
> +    pub swapsize: Option<f64>,
> +    pub maxroot: Option<f64>,
> +    pub maxvz: Option<f64>,
> +    pub minfree: Option<f64>,
> +}
> +
> +impl LvmOptions {
> +    pub fn new() -> LvmOptions {
> +        LvmOptions::default()
> +    }
> +}

^ same as ZfsOptions::new()

> +
> +#[derive(Clone, Copy, Default, Deserialize, Debug)]
> +pub struct BtrfsOptions {
> +    pub hdsize: Option<f64>,
> +    pub raid: Option<BtrfsRaidLevel>,
> +}
> +
> +impl BtrfsOptions {
> +    pub fn new() -> BtrfsOptions {
> +        BtrfsOptions::default()
> +    }
> +}

^ same as ZfsOptions::new()

> diff --git a/proxmox-auto-installer/src/lib.rs b/proxmox-auto-installer/src/lib.rs
> index e69de29..7813b98 100644
> --- a/proxmox-auto-installer/src/lib.rs
> +++ b/proxmox-auto-installer/src/lib.rs
> @@ -0,0 +1 @@
> +pub mod answer;
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>