From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4177CBC95A for ; Fri, 29 Mar 2024 13:37:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 219FA1B10B for ; Fri, 29 Mar 2024 13:37:39 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 29 Mar 2024 13:37:38 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0DAE342B17 for ; Fri, 29 Mar 2024 13:37:38 +0100 (CET) Message-ID: <8909e6da-113a-450b-9389-65fe6415aadc@proxmox.com> Date: Fri, 29 Mar 2024 13:37:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Christoph Heiss Cc: Proxmox VE development discussion References: <20240328135028.504520-1-a.lauterer@proxmox.com> <20240328135028.504520-10-a.lauterer@proxmox.com> Content-Language: en-US From: Aaron Lauterer In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.059 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [answer.rs] Subject: Re: [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Mar 2024 12:37:39 -0000 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 >> --- > [..] >> 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>, >> + pub post_command: Option>, >> + pub reboot_on_error: Option, > > 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, > > Same here as for `reboot_on_error`. > >> + pub cidr: Option, >> + pub dns: Option, >> + pub gateway: Option, >> + // 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>, >> +} >> + > [..] >> + >> +#[derive(Clone, Deserialize, Debug)] >> +pub struct DiskSetup { >> + pub filesystem: Filesystem, >> + pub disk_list: Option>, > > Could this be a > > #[serde(default)] > pub disk_list: Vec, > > 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>, >> + pub filter_match: Option, >> + pub zfs: Option, >> + pub lvm: Option, >> + pub btrfs: Option, >> +} >> + > [..] >> + >> +impl TryFrom for Disks { >> + type Error = &'static str; >> + >> + fn try_from(source: DiskSetup) -> Result { >> + 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), >> + Filter(BTreeMap), >> +} >> +#[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, >> + pub ashift: Option, >> + pub arc_max: Option, >> + pub checksum: Option, >> + pub compress: Option, >> + pub copies: Option, >> + pub hdsize: Option, >> +} >> + >> +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.