[pve-devel] [PATCH installer 0/4] auto-install-assistant: allow specifying 'fetch-from' partition label

[pve-devel] [PATCH installer 0/4] auto-install-assistant: allow specifying 'fetch-from' partition label

[Christoph Heiss]

This series allow specifying the partition label the
`proxmox-fetch-answer` tool will search when trying to retrieve an
answer file in the 'partition' fetch mode.

This has been requested by at least one user one user [0] and definitely
makes sense, esp. for BMCs/IPMIs where one might not be able to control
the partition label.

Patch #1 implements the new CLI parameter in the
`proxmox-auto-install-assistant` tool, patch #4 then integrates it into
the `proxmox-fetch-answer` itself. Patch #2 is just a small (but needed)
refactor of the CLI parsing code.

Patch #3 implements also searching for the partition label in the exact
case it was specified by the user. This is something I stumbled over
while testing it - see the patch itself for some more information.

[0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689

Christoph Heiss (4):
  auto-install-assistant: add new parameter to specify partition label
  fetch-answer: refactor cli argument parsing
  fetch-answer: partition: also search for exact-matching partition
    label
  fetch-answer: use partition label from fetch config instead of
    hardcoded

 proxmox-auto-install-assistant/src/main.rs    | 13 ++++-
 proxmox-auto-installer/src/utils.rs           |  1 +
 .../src/fetch_plugins/partition.rs            | 29 ++++++----
 proxmox-fetch-answer/src/main.rs              | 58 ++++++++++++++-----
 4 files changed, 76 insertions(+), 25 deletions(-)

-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 1/4] auto-install-assistant: add new parameter to specify partition label

[Christoph Heiss]

.. for the 'partition' fetch method.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-auto-install-assistant/src/main.rs | 13 +++++++++++--
 proxmox-auto-installer/src/utils.rs        |  1 +
 proxmox-fetch-answer/src/main.rs           |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 1447175..1f29a45 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -94,8 +94,8 @@ struct CommandValidateAnswer {
 /// The behavior of how to fetch an answer file must be set with the '--fetch-from' parameter. The
 /// answer file can be:{n}
 /// * integrated into the ISO itself ('iso'){n}
-/// * present on a partition / file-system with the label 'PROXMOX-AIS' (Proxmox
-/// Automated Installer Source) ('partition'){n}
+/// * present on a partition / file-system, matched by its label (Proxmox Automated Installer
+///   Source) ('partition'){n}
 /// * requested via an HTTP Post request ('http').
 ///
 /// The URL for the HTTP mode can be defined for the ISO with the '--url' argument. If not present,
@@ -110,6 +110,9 @@ struct CommandValidateAnswer {
 /// to retrieve the URL. For example, the DNS TXT record for the fingerprint will only be used, if
 /// no one was configured with the '--cert-fingerprint' parameter and if the URL was retrieved via
 /// the DNS TXT record.
+///
+/// If the 'partition' mode is used, the '--partition-label' parameter can be used to set the
+/// partition label the auto-installer should search for. This defaults to 'proxmox-ais'.
 #[derive(Args, Debug)]
 struct CommandPrepareISO {
     /// Path to the source ISO to prepare
@@ -141,6 +144,11 @@ struct CommandPrepareISO {
     /// input ISO file.
     #[arg(long)]
     tmp: Option<String>,
+
+    /// Can be used in combination with `--fetch-from partition` to set the partition label
+    /// the auto-installer will search for.
+    #[arg(long, default_value_t = { "proxmox-ais".to_owned() } )]
+    partition_label: String,
 }
 
 /// Show the system information that can be used to identify a host.
@@ -323,6 +331,7 @@ fn prepare_iso(args: &CommandPrepareISO) -> Result<()> {
     println!("Preparing ISO...");
     let config = AutoInstSettings {
         mode: args.fetch_from.clone(),
+        partition_label: args.partition_label.clone(),
         http: HttpOptions {
             url: args.url.clone(),
             cert_fingerprint: args.cert_fingerprint.clone(),
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 45ad222..8ff8134 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -86,6 +86,7 @@ pub struct HttpOptions {
 #[serde(rename_all = "lowercase", deny_unknown_fields)]
 pub struct AutoInstSettings {
     pub mode: FetchAnswerFrom,
+    pub partition_label: String,
     #[serde(default)]
     pub http: HttpOptions,
 }
diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 660dc51..86c3270 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -63,6 +63,7 @@ fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
     }
     Ok(AutoInstSettings {
         mode,
+        partition_label: "proxmox-ais".to_owned(),
         http: HttpOptions {
             url: args.get(2).cloned(),
             cert_fingerprint: args.get(3).cloned(),
-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 2/4] fetch-answer: refactor cli argument parsing

[Christoph Heiss]

Clean up the actual parsing a bit to make it more easily extensible, and
add a proper help menu.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-fetch-answer/src/main.rs | 40 +++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 86c3270..753ab29 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -16,6 +16,22 @@ mod fetch_plugins;
 static LOGGER: AutoInstLogger = AutoInstLogger;
 static AUTOINST_MODE_FILE: &str = "/cdrom/auto-installer-mode.toml";
 
+const CLI_USAGE_HELPTEXT: &str = concat!(
+    "Usage: ",
+    env!("CARGO_BIN_NAME"),
+    " <command> <additional parameters..>
+
+Commands:
+  iso         Fetch the builtin answer file from the ISO
+  http        Fetch the answer file via HTTP(S)
+              Additional parameters: [<http-url>] [<tls-cert-fingerprint>]
+  partition   Fetch the answer file from a mountable partition
+
+Options:
+  -h, --help  Print this help menu
+"
+);
+
 pub fn init_log() -> Result<()> {
     AutoInstLogger::init("/tmp/fetch_answer.log")?;
     log::set_logger(&LOGGER)
@@ -46,21 +62,27 @@ fn fetch_answer(install_settings: &AutoInstSettings) -> Result<String> {
 }
 
 fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
-    // TODO: this was done in a bit of a hurry, needs tidying up
     let mode = match args[1].to_lowercase().as_str() {
         "iso" => FetchAnswerFrom::Iso,
         "http" => FetchAnswerFrom::Http,
         "partition" => FetchAnswerFrom::Partition,
-        "-h" | "--help" => bail!(
-            "usage: {} <http|iso|partition> [<http-url>] [<tls-cert-fingerprint>]",
-            args[0]
-        ),
+        "-h" | "--help" => {
+            eprintln!("{}", CLI_USAGE_HELPTEXT);
+            bail!("invalid usage");
+        }
         _ => bail!("failed to parse fetch-from argument, not one of 'http', 'iso', or 'partition'"),
     };
-    if args.len() > 4 {
-    } else if args.len() > 2 && mode != FetchAnswerFrom::Http {
-        bail!("only 'http' fetch-from mode supports additional url and cert-fingerprint mode");
-    }
+
+    match mode {
+        FetchAnswerFrom::Iso | FetchAnswerFrom::Partition if args.len() > 2 => {
+            bail!("'iso' and 'partition' modes do not take any additional arguments")
+        }
+        FetchAnswerFrom::Http if args.len() > 4 => {
+            bail!("'http' mode takes at most 2 additional arguments")
+        }
+        _ => {}
+    };
+
     Ok(AutoInstSettings {
         mode,
         partition_label: "proxmox-ais".to_owned(),
-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 3/4] fetch-answer: partition: also search for exact-matching partition label

[Christoph Heiss]

While some filesystems - such as FAT(32) - might not supported/allow
mixed-case labels, some implementations still handle them correctly,
such as Linux. Thus, also search for that variant.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/fetch_plugins/partition.rs               | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
index 4472922..cbfe2d5 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
@@ -43,12 +43,22 @@ fn path_exists_logged(file_name: &str, search_path: &str) -> Option<PathBuf> {
     }
 }
 
-/// Searches for upper and lower case existence of the partlabel in the search_path
+/// Searches for the exact case, upper and finally lower case existence of the partlabel in the
+/// search_path, in that order.
+///
+/// While some filesystems - such as FAT(32) - might not supported/allow mixed-case labels, some
+/// implementations still handle them correctly, such as Linux. Thus, also search for that variant
+/// first.
 ///
 /// # Arguments
-/// * `partlabel_source` - Partition Label, used as upper and lower case
+/// * `partlabel_source` - Partition Label, used for matching, in the exact, upper and lower case
 /// * `search_path` - Path where to search for the partition label
 fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
+    if let Some(path) = path_exists_logged(partlabel, search_path) {
+        info!("Found partition with label '{partlabel}'");
+        return Ok(path);
+    }
+
     let partlabel_upper_case = partlabel.to_uppercase();
     if let Some(path) = path_exists_logged(&partlabel_upper_case, search_path) {
         info!("Found partition with label '{partlabel_upper_case}'");
@@ -61,7 +71,7 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
         return Ok(path);
     }
 
-    bail!("Could not detect upper or lower case labels for '{partlabel}'");
+    bail!("Could not find partition for label '{partlabel}'");
 }
 
 /// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Christoph Heiss]

This has been requested by at least one user one user [0] and definitely
makes sense, esp. for BMCs/IPMIs where one might not be able to control
the partition label.

[0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/fetch_plugins/partition.rs            | 13 +++++------
 proxmox-fetch-answer/src/main.rs              | 23 +++++++++++++------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
index cbfe2d5..c68dc59 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
@@ -9,17 +9,16 @@ use std::{
 static ANSWER_FILE: &str = "answer.toml";
 static ANSWER_MP: &str = "/mnt/answer";
 // FAT can only handle 11 characters, so shorten Automated Installer Source to AIS
-static PARTLABEL: &str = "proxmox-ais";
 static DISK_BY_ID_PATH: &str = "/dev/disk/by-label";
 
 pub struct FetchFromPartition;
 
 impl FetchFromPartition {
     /// Returns the contents of the answer file
-    pub fn get_answer() -> Result<String> {
+    pub fn get_answer(part_label: &str) -> Result<String> {
         info!("Checking for answer file on partition.");
 
-        let mut mount_path = PathBuf::from(mount_proxmoxinst_part()?);
+        let mut mount_path = PathBuf::from(mount_proxmoxinst_part(part_label)?);
         mount_path.push(ANSWER_FILE);
         let answer = fs::read_to_string(mount_path)
             .map_err(|err| format_err!("failed to read answer file - {err}"))?;
@@ -74,14 +73,14 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
     bail!("Could not find partition for label '{partlabel}'");
 }
 
-/// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
-/// to ANSWER_MP
-fn mount_proxmoxinst_part() -> Result<String> {
+/// Will search and mount a partition/FS labeled labeled `part_label` in lower or uppercase to
+/// ANSWER_MP
+fn mount_proxmoxinst_part(part_label: &str) -> Result<String> {
     if let Ok(true) = check_if_mounted(ANSWER_MP) {
         info!("Skipping: '{ANSWER_MP}' is already mounted.");
         return Ok(ANSWER_MP.into());
     }
-    let part_path = scan_partlabels(PARTLABEL, DISK_BY_ID_PATH)?;
+    let part_path = scan_partlabels(part_label, DISK_BY_ID_PATH)?;
     info!("Mounting partition at {ANSWER_MP}");
     // create dir for mountpoint
     create_dir_all(ANSWER_MP)?;
diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 753ab29..d56f8b9 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -26,6 +26,7 @@ Commands:
   http        Fetch the answer file via HTTP(S)
               Additional parameters: [<http-url>] [<tls-cert-fingerprint>]
   partition   Fetch the answer file from a mountable partition
+              Additional parameters: [<partition-label>]
 
 Options:
   -h, --help  Print this help menu
@@ -49,10 +50,12 @@ fn fetch_answer(install_settings: &AutoInstSettings) -> Result<String> {
                 Err(err) => info!("Fetching answer file from ISO failed: {err}"),
             }
         }
-        FetchAnswerFrom::Partition => match FetchFromPartition::get_answer() {
-            Ok(answer) => return Ok(answer),
-            Err(err) => info!("Fetching answer file from partition failed: {err}"),
-        },
+        FetchAnswerFrom::Partition => {
+            match FetchFromPartition::get_answer(&install_settings.partition_label) {
+                Ok(answer) => return Ok(answer),
+                Err(err) => info!("Fetching answer file from partition failed: {err}"),
+            }
+        }
         FetchAnswerFrom::Http => match FetchFromHTTP::get_answer(&install_settings.http) {
             Ok(answer) => return Ok(answer),
             Err(err) => info!("Fetching answer file via HTTP failed: {err}"),
@@ -74,18 +77,24 @@ fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
     };
 
     match mode {
-        FetchAnswerFrom::Iso | FetchAnswerFrom::Partition if args.len() > 2 => {
-            bail!("'iso' and 'partition' modes do not take any additional arguments")
+        FetchAnswerFrom::Iso if args.len() > 2 => {
+            bail!("'iso' mode does not take any additional arguments")
         }
         FetchAnswerFrom::Http if args.len() > 4 => {
             bail!("'http' mode takes at most 2 additional arguments")
         }
+        FetchAnswerFrom::Partition if args.len() > 3 => {
+            bail!("'partition' mode takes at most 1 additional argument")
+        }
         _ => {}
     };
 
     Ok(AutoInstSettings {
         mode,
-        partition_label: "proxmox-ais".to_owned(),
+        partition_label: args
+            .get(2)
+            .ok_or(format_err!("partition label expected"))
+            .cloned()?,
         http: HttpOptions {
             url: args.get(2).cloned(),
             cert_fingerprint: args.get(3).cloned(),
-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Aaron Lauterer]

only small nits inline

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-10-18  13:59, Christoph Heiss wrote:
> This has been requested by at least one user one user [0] and definitely
> makes sense, esp. for BMCs/IPMIs where one might not be able to control
> the partition label.
> 
> [0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   .../src/fetch_plugins/partition.rs            | 13 +++++------
>   proxmox-fetch-answer/src/main.rs              | 23 +++++++++++++------
>   2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> index cbfe2d5..c68dc59 100644
> --- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> +++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> @@ -9,17 +9,16 @@ use std::{
>   static ANSWER_FILE: &str = "answer.toml";
>   static ANSWER_MP: &str = "/mnt/answer";
>   // FAT can only handle 11 characters, so shorten Automated Installer Source to AIS

this comment is now dangling. we could move that to the definition of 
the default value for the new parameter

> -static PARTLABEL: &str = "proxmox-ais";
>   static DISK_BY_ID_PATH: &str = "/dev/disk/by-label";
>   
>   pub struct FetchFromPartition;
>   
>   impl FetchFromPartition {
>       /// Returns the contents of the answer file
> -    pub fn get_answer() -> Result<String> {
> +    pub fn get_answer(part_label: &str) -> Result<String> {
>           info!("Checking for answer file on partition.");
>   
> -        let mut mount_path = PathBuf::from(mount_proxmoxinst_part()?);
> +        let mut mount_path = PathBuf::from(mount_proxmoxinst_part(part_label)?);
>           mount_path.push(ANSWER_FILE);
>           let answer = fs::read_to_string(mount_path)
>               .map_err(|err| format_err!("failed to read answer file - {err}"))?;
> @@ -74,14 +73,14 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
>       bail!("Could not find partition for label '{partlabel}'");
>   }
>   
> -/// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
> -/// to ANSWER_MP
> -fn mount_proxmoxinst_part() -> Result<String> {
> +/// Will search and mount a partition/FS labeled labeled `part_label` in lower or uppercase to

one `labeled` too much :)

> +/// ANSWER_MP
> +fn mount_proxmoxinst_part(part_label: &str) -> Result<String> {
>       if let Ok(true) = check_if_mounted(ANSWER_MP) {
>           info!("Skipping: '{ANSWER_MP}' is already mounted.");[…]


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 0/4] auto-install-assistant: allow specifying 'fetch-from' partition label

[Aaron Lauterer]

except for two small style nits in comments in patch 4/4 I don't have 
anything to complain about.

Consider this series

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-10-18  13:59, Christoph Heiss wrote:
> This series allow specifying the partition label the
> `proxmox-fetch-answer` tool will search when trying to retrieve an
> answer file in the 'partition' fetch mode.
> 
> This has been requested by at least one user one user [0] and definitely
> makes sense, esp. for BMCs/IPMIs where one might not be able to control
> the partition label.
> 
> Patch #1 implements the new CLI parameter in the
> `proxmox-auto-install-assistant` tool, patch #4 then integrates it into
> the `proxmox-fetch-answer` itself. Patch #2 is just a small (but needed)
> refactor of the CLI parsing code.
> 
> Patch #3 implements also searching for the partition label in the exact
> case it was specified by the user. This is something I stumbled over
> while testing it - see the patch itself for some more information.
> 
> [0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689
> 
> Christoph Heiss (4):
>    auto-install-assistant: add new parameter to specify partition label
>    fetch-answer: refactor cli argument parsing
>    fetch-answer: partition: also search for exact-matching partition
>      label
>    fetch-answer: use partition label from fetch config instead of
>      hardcoded
> 
>   proxmox-auto-install-assistant/src/main.rs    | 13 ++++-
>   proxmox-auto-installer/src/utils.rs           |  1 +
>   .../src/fetch_plugins/partition.rs            | 29 ++++++----
>   proxmox-fetch-answer/src/main.rs              | 58 ++++++++++++++-----
>   4 files changed, 76 insertions(+), 25 deletions(-)
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Christoph Heiss]

Thanks for the review!

On Thu, Nov 07, 2024 at 04:28:25PM +0100, Aaron Lauterer wrote:
> only small nits inline
>
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
>
> On  2024-10-18  13:59, Christoph Heiss wrote:
> > [..]
> > diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> > index cbfe2d5..c68dc59 100644
> > --- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> > +++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
> > @@ -9,17 +9,16 @@ use std::{
> >   static ANSWER_FILE: &str = "answer.toml";
> >   static ANSWER_MP: &str = "/mnt/answer";
> >   // FAT can only handle 11 characters, so shorten Automated Installer Source to AIS
>
> this comment is now dangling. we could move that to the definition of the
> default value for the new parameter

Yep, good catch!

Also, while reading this, seems nowhere is checked whether the
user-supplied value is at max 11 characters long -- I'll add that for v2
too.

> > [..]
> > @@ -74,14 +73,14 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
> >       bail!("Could not find partition for label '{partlabel}'");
> >   }
> > -/// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
> > -/// to ANSWER_MP
> > -fn mount_proxmoxinst_part() -> Result<String> {
> > +/// Will search and mount a partition/FS labeled labeled `part_label` in lower or uppercase to
>
> one `labeled` too much :)

Thx!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Aaron Lauterer]

On  2024-11-08  10:28, Christoph Heiss wrote:
> Thanks for the review!
> 
> On Thu, Nov 07, 2024 at 04:28:25PM +0100, Aaron Lauterer wrote:
>> only small nits inline
>>
>> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
>> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
>>
>> On  2024-10-18  13:59, Christoph Heiss wrote:
>>> [..]
>>> diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
>>> index cbfe2d5..c68dc59 100644
>>> --- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
>>> +++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
>>> @@ -9,17 +9,16 @@ use std::{
>>>    static ANSWER_FILE: &str = "answer.toml";
>>>    static ANSWER_MP: &str = "/mnt/answer";
>>>    // FAT can only handle 11 characters, so shorten Automated Installer Source to AIS
>>
>> this comment is now dangling. we could move that to the definition of the
>> default value for the new parameter
> 
> Yep, good catch!
> 
> Also, while reading this, seems nowhere is checked whether the
> user-supplied value is at max 11 characters long -- I'll add that for v2
> too.

Too be honest, do we need to do that? For the default it is important, 
but users can use other FS that might allow for longer labels and if the 
kernel of the install live system can handle it, all is good.
> 
>>> [..]
>>> @@ -74,14 +73,14 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
>>>        bail!("Could not find partition for label '{partlabel}'");
>>>    }
>>> -/// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
>>> -/// to ANSWER_MP
>>> -fn mount_proxmoxinst_part() -> Result<String> {
>>> +/// Will search and mount a partition/FS labeled labeled `part_label` in lower or uppercase to
>>
>> one `labeled` too much :)
> 
> Thx!



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Christoph Heiss]

On Fri, Nov 08, 2024 at 10:30:18AM +0100, Aaron Lauterer wrote:
> On  2024-11-08  10:28, Christoph Heiss wrote:
> > [..]
> > Also, while reading this, seems nowhere is checked whether the
> > user-supplied value is at max 11 characters long -- I'll add that for v2
> > too.
>
> Too be honest, do we need to do that? For the default it is important, but
> users can use other FS that might allow for longer labels and if the kernel
> of the install live system can handle it, all is good.

Right, we aren't really constrained by FAT here! Thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel