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