From: Christoph Heiss <c.heiss@proxmox.com>
To: Aaron Lauterer <a.lauterer@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition
Date: Fri, 29 Mar 2024 12:43:08 +0100 [thread overview]
Message-ID: <c534t2gblwxpkmkkqh3h3ditqk4lbsymysnc3uhxcwb57e6ee4@dnofn2u6nzhr> (raw)
In-Reply-To: <20240328135028.504520-10-a.lauterer@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
>
>
next prev parent reply other threads:[~2024-03-29 11:43 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 13:49 [pve-devel] [PATCH v3 00/30] add automated/unattended installation Aaron Lauterer
2024-03-28 13:49 ` [pve-devel] [PATCH v3 01/30] tui: common: move InstallConfig struct to common crate Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 02/30] common: make InstallZfsOption members public Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 03/30] common: tui: use BTreeMap for predictable ordering Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 04/30] common: utils: add deserializer for CidrAddress Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 05/30] common: options: add Deserialize trait Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 06/30] low-level: add dump-udev command Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 07/30] add auto-installer crate Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 08/30] auto-installer: add dependencies Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition Aaron Lauterer
2024-03-29 11:43 ` Christoph Heiss [this message]
2024-03-29 12:37 ` Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 10/30] auto-installer: add struct to hold udev info Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 11/30] auto-installer: add utils Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 12/30] auto-installer: add simple logging Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 13/30] auto-installer: add tests for answer file parsing Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 14/30] auto-installer: add auto-installer binary Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 15/30] auto-installer: add fetch answer binary Aaron Lauterer
2024-04-02 12:03 ` Christoph Heiss
2024-03-28 13:50 ` [pve-devel] [PATCH v3 16/30] unconfigured: add proxauto as option to start auto installer Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 17/30] auto-installer: use glob crate for pattern matching Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 18/30] auto-installer: utils: make get_udev_index functions public Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 19/30] auto-installer: add proxmox-autoinst-helper tool Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 20/30] auto-installer: fetch: add gathering of system identifiers and restructure code Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 21/30] auto-installer: helper: add subcommand to view indentifiers Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 22/30] auto-installer: fetch: add http post utility module Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 23/30] auto-installer: fetch: add http plugin to fetch answer Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 24/30] control: update build depends for auto installer Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 25/30] auto installer: factor out fetch-answer and autoinst-helper Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 26/30] low-level: write low level config to /tmp Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 27/30] common: add deserializer for FsType Aaron Lauterer
2024-03-29 12:20 ` Christoph Heiss
2024-03-29 12:38 ` Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 28/30] common: skip target_hd when deserializing InstallConfig Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 29/30] common: add Display trait to ProxmoxProduct Aaron Lauterer
2024-03-28 13:50 ` [pve-devel] [PATCH v3 30/30] add proxmox-chroot utility Aaron Lauterer
2024-03-28 13:53 ` [pve-devel] [PATCH v3 00/30] add automated/unattended installation Aaron Lauterer
2024-04-02 14:43 ` Christoph Heiss
2024-04-02 14:55 ` Aaron Lauterer
2024-04-03 8:19 ` Christoph Heiss
2024-04-03 8:47 ` Aaron Lauterer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c534t2gblwxpkmkkqh3h3ditqk4lbsymysnc3uhxcwb57e6ee4@dnofn2u6nzhr \
--to=c.heiss@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.