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.laute...@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
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
