[pbs-devel] [PATCH v4 proxmox{, -backup} 0/3] Add boot_mode, improve kernel version

[pbs-devel] [PATCH v4 proxmox{, -backup} 0/3] Add boot_mode, improve kernel version

[Gabriel Goller]

Ported the recent changes from the PVE NodeSummary (done by @Thomas) to
the PBS NodeDashboard.

It consists of:
* Adding the bootmode field, shows either Legacy BIOS, EFI, or EFI
    (Secure Boot)
* Declutter the kernel-version field and only show the release version
    and build-date.

Changes since v3:
  * Removed leftover debug print

Changes since v2:
 * Return the exact same stuff as in pve, so create a struct that holds
    mode and secureboot boolean
 * Fix indentation in js
 * Add efi spec link

Changes since v1:
 * Moved boot_mode detection to proxmox-sys
 * Added caching to boot_mode detection (lazy_static)
 * Return legacy kernel-version as well



proxmox:

Gabriel Goller (1):
  sys: add function to get boot_mode

 proxmox-sys/src/boot_mode.rs | 54 ++++++++++++++++++++++++++++++++++++
 proxmox-sys/src/lib.rs       |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 proxmox-sys/src/boot_mode.rs


proxmox-backup:

Gabriel Goller (2):
  node: status: added bootmode
  node: status: declutter kernel-version

 pbs-api-types/src/node.rs | 76 +++++++++++++++++++++++++++++++++++++--
 src/api2/node/status.rs   | 40 ++++++++++++++++-----
 www/panel/NodeInfo.js     | 28 +++++++++++++--
 3 files changed, 131 insertions(+), 13 deletions(-)


Summary over all repositories:
  5 files changed, 186 insertions(+), 13 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH v4 proxmox 1/3] sys: add function to get boot_mode

[Gabriel Goller]

Helper that returns the current boot_mode. Either EFI, BIOS, or EFI
(Secure Boot).
Detection works the same as in pve, we use `/sys/firmware/efi` and
the `efivars/SecureBoot-xxx..` file.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-sys/src/boot_mode.rs | 54 ++++++++++++++++++++++++++++++++++++
 proxmox-sys/src/lib.rs       |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 proxmox-sys/src/boot_mode.rs

diff --git a/proxmox-sys/src/boot_mode.rs b/proxmox-sys/src/boot_mode.rs
new file mode 100644
index 0000000..6dcdf07
--- /dev/null
+++ b/proxmox-sys/src/boot_mode.rs
@@ -0,0 +1,54 @@
+use std::{io::Read, sync::Mutex};
+
+#[derive(Clone, Copy)]
+pub enum SecureBoot {
+    /// SecureBoot is enabled
+    Enabled,
+    /// SecureBoot is disabled
+    Disabled,
+}
+
+/// The possible BootModes
+#[derive(Clone, Copy)]
+pub enum BootModeInformation {
+    /// The BootMode is EFI/UEFI, has a SecureBoot variant
+    Efi(SecureBoot),
+    /// The BootMode is Legacy BIOS
+    Bios,
+}
+
+// Returns the current bootmode (BIOS, EFI, or EFI(Secure Boot))
+pub fn boot_mode() -> BootModeInformation {
+    lazy_static::lazy_static!(
+        static ref BOOT_MODE: Mutex<Option<BootModeInformation>> = Mutex::new(None);
+    );
+
+    let mut last = BOOT_MODE.lock().unwrap();
+    let value = last.or_else(|| {
+        if std::path::Path::new("/sys/firmware/efi").exists() {
+            // Check if SecureBoot is enabled
+            // Attention: this file is not seekable!
+            // Spec: https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html?highlight=8be4d#globally-defined-variables
+            let efivar = std::fs::File::open(
+                "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
+            );
+            if let Ok(mut file) = efivar {
+                let mut buf = [0; 5];
+                let Ok(_) = file.read_exact(&mut buf) else {
+                    return Some(BootModeInformation::Efi(SecureBoot::Disabled));
+                };
+                if buf[4..] == [1] {
+                    Some(BootModeInformation::Efi(SecureBoot::Enabled))
+                } else {
+                    Some(BootModeInformation::Efi(SecureBoot::Disabled))
+                }
+            } else {
+                Some(BootModeInformation::Efi(SecureBoot::Disabled))
+            }
+        } else {
+            Some(BootModeInformation::Bios)
+        }
+    });
+    *last = value;
+    value.unwrap()
+}
diff --git a/proxmox-sys/src/lib.rs b/proxmox-sys/src/lib.rs
index 7e59058..8ea7073 100644
--- a/proxmox-sys/src/lib.rs
+++ b/proxmox-sys/src/lib.rs
@@ -1,5 +1,6 @@
 use std::os::unix::ffi::OsStrExt;
 
+pub mod boot_mode;
 pub mod command;
 #[cfg(feature = "crypt")]
 pub mod crypt;
-- 
2.39.2

Re: [pbs-devel] [PATCH v4 proxmox 1/3] sys: add function to get boot_mode

[Wolfgang Bumiller]

On Mon, Nov 27, 2023 at 11:52:36AM +0100, Gabriel Goller wrote:
> Helper that returns the current boot_mode. Either EFI, BIOS, or EFI
> (Secure Boot).
> Detection works the same as in pve, we use `/sys/firmware/efi` and
> the `efivars/SecureBoot-xxx..` file.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  proxmox-sys/src/boot_mode.rs | 54 ++++++++++++++++++++++++++++++++++++
>  proxmox-sys/src/lib.rs       |  1 +
>  2 files changed, 55 insertions(+)
>  create mode 100644 proxmox-sys/src/boot_mode.rs
> 
> diff --git a/proxmox-sys/src/boot_mode.rs b/proxmox-sys/src/boot_mode.rs
> new file mode 100644
> index 0000000..6dcdf07
> --- /dev/null
> +++ b/proxmox-sys/src/boot_mode.rs
> @@ -0,0 +1,54 @@
> +use std::{io::Read, sync::Mutex};
> +
> +#[derive(Clone, Copy)]
> +pub enum SecureBoot {
> +    /// SecureBoot is enabled
> +    Enabled,
> +    /// SecureBoot is disabled
> +    Disabled,
> +}
> +
> +/// The possible BootModes
> +#[derive(Clone, Copy)]
> +pub enum BootModeInformation {
> +    /// The BootMode is EFI/UEFI, has a SecureBoot variant
> +    Efi(SecureBoot),
> +    /// The BootMode is Legacy BIOS
> +    Bios,
> +}
> +
> +// Returns the current bootmode (BIOS, EFI, or EFI(Secure Boot))
> +pub fn boot_mode() -> BootModeInformation {

In addition to my v3 reply with the range comparison: please avoid more
free-standing functions for this type of stuff.

Instead, this could be BootModeInformation::query().

Also, are we sure we want this a a nested enum if we separate it in
`pbs-api-types` later? Maybe these should just stay separate things,
queried via BootMode::query() + SecureBoot::query()
(A bit shorter with the `Information` suffix dropped there...)

Re: [pbs-devel] [PATCH v4 proxmox 1/3] sys: add function to get boot_mode

[Gabriel Goller]

Thanks for the review!

On 11/27/23 14:08, Wolfgang Bumiller wrote:
> On Mon, Nov 27, 2023 at 11:52:36AM +0100, Gabriel Goller wrote:
>> Helper that returns the current boot_mode. Either EFI, BIOS, or EFI
>> (Secure Boot).
>> Detection works the same as in pve, we use `/sys/firmware/efi` and
>> the `efivars/SecureBoot-xxx..` file.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   proxmox-sys/src/boot_mode.rs | 54 ++++++++++++++++++++++++++++++++++++
>>   proxmox-sys/src/lib.rs       |  1 +
>>   2 files changed, 55 insertions(+)
>>   create mode 100644 proxmox-sys/src/boot_mode.rs
>>
>> diff --git a/proxmox-sys/src/boot_mode.rs b/proxmox-sys/src/boot_mode.rs
>> new file mode 100644
>> index 0000000..6dcdf07
>> --- /dev/null
>> +++ b/proxmox-sys/src/boot_mode.rs
>> @@ -0,0 +1,54 @@
>> +use std::{io::Read, sync::Mutex};
>> +
>> +#[derive(Clone, Copy)]
>> +pub enum SecureBoot {
>> +    /// SecureBoot is enabled
>> +    Enabled,
>> +    /// SecureBoot is disabled
>> +    Disabled,
>> +}
>> +
>> +/// The possible BootModes
>> +#[derive(Clone, Copy)]
>> +pub enum BootModeInformation {
>> +    /// The BootMode is EFI/UEFI, has a SecureBoot variant
>> +    Efi(SecureBoot),
>> +    /// The BootMode is Legacy BIOS
>> +    Bios,
>> +}
>> +
>> +// Returns the current bootmode (BIOS, EFI, or EFI(Secure Boot))
>> +pub fn boot_mode() -> BootModeInformation {
> In addition to my v3 reply with the range comparison: please avoid more
> free-standing functions for this type of stuff.
>
> Instead, this could be BootModeInformation::query().
>
> Also, are we sure we want this a a nested enum if we separate it in
> `pbs-api-types` later? Maybe these should just stay separate things,
> queried via BootMode::query() + SecureBoot::query()
> (A bit shorter with the `Information` suffix dropped there...)
Oh, that's actually not a bad idea.
I would have one `BootMode::query()` function that just checks if the 
`/sys/firmare/efi`
dir exists and return `Bios` or `Efi` + another function 
`SecureBoot::query()` that
returns `Enabled` or `Disabled` if the secureboot is enabled (checking 
the SecureBoot-xxx efivar).

[pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

Added field that shows the bootmode of the node. The bootmode is either
Legacy Bios, EFI, or EFI (Secure Boot). To detect the mode we use the
exact same method as in pve: We check if the `/sys/firmware/efi` folder
exists, then check if the `SecureBoot-xx...` file in the `efivars`
directory has the SecureBoot flag enabled.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-api-types/src/node.rs | 29 +++++++++++++++++++++++++++--
 src/api2/node/status.rs   | 28 ++++++++++++++++++++++++++--
 www/panel/NodeInfo.js     | 15 +++++++++++++++
 3 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
index 704215bb..8bcf8fd6 100644
--- a/pbs-api-types/src/node.rs
+++ b/pbs-api-types/src/node.rs
@@ -1,9 +1,8 @@
-use serde::{Deserialize, Serialize};
 use proxmox_schema::*;
+use serde::{Deserialize, Serialize};
 
 use crate::StorageStatus;
 
-
 #[api]
 #[derive(Serialize, Deserialize, Default)]
 #[serde(rename_all = "kebab-case")]
@@ -39,6 +38,30 @@ pub struct NodeInformation {
     pub fingerprint: String,
 }
 
+
+#[api]
+#[derive(Serialize, Deserialize, Default)]
+#[serde(rename_all = "kebab-case")]
+/// The possible BootModes
+pub enum BootMode {
+    /// The BootMode is EFI/UEFI
+    Efi,
+    /// The BootMode is Legacy BIOS
+    #[default]
+    LegacyBios,
+}
+
+#[api]
+#[derive(Serialize, Deserialize, Default)]
+#[serde(rename_all = "lowercase")]
+/// Holds the Bootmodes
+pub struct BootModeInformation {
+    /// The BootMode, either Efi or Bios
+    pub mode: BootMode,
+    /// SecureBoot status
+    pub secureboot: bool,
+}
+
 #[api]
 #[derive(Serialize, Deserialize, Default)]
 #[serde(rename_all = "kebab-case")]
@@ -97,4 +120,6 @@ pub struct NodeStatus {
     pub wait: f64,
     pub cpuinfo: NodeCpuInformation,
     pub info: NodeInformation,
+    /// Current boot mode
+    pub boot_info: BootModeInformation,
 }
diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
index 639d7211..21f1bc2c 100644
--- a/src/api2/node/status.rs
+++ b/src/api2/node/status.rs
@@ -1,16 +1,18 @@
-use std::os::unix::prelude::OsStrExt;
+use std::os::unix::ffi::OsStrExt;
 use std::process::Command;
 
 use anyhow::{bail, format_err, Error};
 use serde_json::Value;
 
+use proxmox_sys::boot_mode;
 use proxmox_sys::linux::procfs;
 
 use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_POWER_MANAGEMENT,
+    BootModeInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT,
+    PRIV_SYS_POWER_MANAGEMENT,
 };
 
 use pbs_api_types::{
@@ -25,6 +27,25 @@ fn procfs_to_node_cpu_info(info: procfs::ProcFsCPUInfo) -> NodeCpuInformation {
     }
 }
 
+fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> BootModeInformation {
+    match bm {
+        boot_mode::BootModeInformation::Efi(secureboot) => match secureboot {
+            boot_mode::SecureBoot::Enabled => BootModeInformation {
+                mode: pbs_api_types::BootMode::Efi,
+                secureboot: true,
+            },
+            boot_mode::SecureBoot::Disabled => BootModeInformation {
+                mode: pbs_api_types::BootMode::Efi,
+                secureboot: false,
+            },
+        },
+        boot_mode::BootModeInformation::Bios => BootModeInformation {
+            mode: pbs_api_types::BootMode::LegacyBios,
+            secureboot: false,
+        },
+    }
+}
+
 #[api(
     input: {
         properties: {
@@ -79,6 +100,8 @@ async fn get_status(
 
     let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
 
+    let boot_info = boot_mode_to_info(boot_mode::boot_mode());
+
     Ok(NodeStatus {
         memory,
         swap,
@@ -96,6 +119,7 @@ async fn get_status(
         info: NodeInformation {
             fingerprint: crate::cert_info()?.fingerprint()?,
         },
+        boot_info,
     })
 }
 
diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
index 2551c9a5..cba6d2a1 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -147,6 +147,21 @@ Ext.define('PBS.NodeInfoPanel', {
 	    textField: 'kversion',
 	    value: '',
 	},
+	{
+	    colspan: 2,
+	    title: gettext('Boot Mode'),
+	    printBar: false,
+	    textField: 'boot-info',
+	    renderer: boot => {
+		if (boot.mode === 'legacy-bios') {
+		    return 'Legacy BIOS';
+		} else if (boot.mode === 'efi') {
+		    return `EFI${boot.secureboot ? ' (Secure Boot)' : ''}`;
+		}
+		return Proxmox.Utils.unknownText;
+	    },
+	    value: '',
+	},
 	{
 	    xtype: 'pmxNodeInfoRepoStatus',
 	    itemId: 'repositoryStatus',
-- 
2.39.2

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Wolfgang Bumiller]

On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
> Added field that shows the bootmode of the node. The bootmode is either
> Legacy Bios, EFI, or EFI (Secure Boot). To detect the mode we use the
> exact same method as in pve: We check if the `/sys/firmware/efi` folder
> exists, then check if the `SecureBoot-xx...` file in the `efivars`
> directory has the SecureBoot flag enabled.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-api-types/src/node.rs | 29 +++++++++++++++++++++++++++--
>  src/api2/node/status.rs   | 28 ++++++++++++++++++++++++++--
>  www/panel/NodeInfo.js     | 15 +++++++++++++++
>  3 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
> index 704215bb..8bcf8fd6 100644
> --- a/pbs-api-types/src/node.rs
> +++ b/pbs-api-types/src/node.rs
> @@ -1,9 +1,8 @@
> -use serde::{Deserialize, Serialize};
>  use proxmox_schema::*;
> +use serde::{Deserialize, Serialize};
>  
>  use crate::StorageStatus;
>  
> -
>  #[api]
>  #[derive(Serialize, Deserialize, Default)]
>  #[serde(rename_all = "kebab-case")]
> @@ -39,6 +38,30 @@ pub struct NodeInformation {
>      pub fingerprint: String,
>  }
>  
> +
> +#[api]
> +#[derive(Serialize, Deserialize, Default)]

And Clone + Copy

> +#[serde(rename_all = "kebab-case")]
> +/// The possible BootModes
> +pub enum BootMode {
> +    /// The BootMode is EFI/UEFI
> +    Efi,
> +    /// The BootMode is Legacy BIOS
> +    #[default]

^ do we *need* Default on this type? And why is Bios the default?

> +    LegacyBios,
> +}
> +
> +#[api]
> +#[derive(Serialize, Deserialize, Default)]

And Clone

> +#[serde(rename_all = "lowercase")]
> +/// Holds the Bootmodes
> +pub struct BootModeInformation {
> +    /// The BootMode, either Efi or Bios
> +    pub mode: BootMode,
> +    /// SecureBoot status
> +    pub secureboot: bool,
> +}
> +
>  #[api]
>  #[derive(Serialize, Deserialize, Default)]
>  #[serde(rename_all = "kebab-case")]
> @@ -97,4 +120,6 @@ pub struct NodeStatus {
>      pub wait: f64,
>      pub cpuinfo: NodeCpuInformation,
>      pub info: NodeInformation,
> +    /// Current boot mode
> +    pub boot_info: BootModeInformation,
>  }
> diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
> index 639d7211..21f1bc2c 100644
> --- a/src/api2/node/status.rs
> +++ b/src/api2/node/status.rs
> @@ -1,16 +1,18 @@
> -use std::os::unix::prelude::OsStrExt;
> +use std::os::unix::ffi::OsStrExt;
>  use std::process::Command;
>  
>  use anyhow::{bail, format_err, Error};
>  use serde_json::Value;
>  
> +use proxmox_sys::boot_mode;
>  use proxmox_sys::linux::procfs;
>  
>  use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
>  use proxmox_schema::api;
>  
>  use pbs_api_types::{
> -    NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_POWER_MANAGEMENT,
> +    BootModeInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT,
> +    PRIV_SYS_POWER_MANAGEMENT,
>  };
>  
>  use pbs_api_types::{
> @@ -25,6 +27,25 @@ fn procfs_to_node_cpu_info(info: procfs::ProcFsCPUInfo) -> NodeCpuInformation {
>      }
>  }
>  
> +fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> BootModeInformation {

I'd prefer the match to be a single level by doing

    use boot_mode::{BootModeIntofmation::*, SecureBoot};
    use pbs_api_types::BootMode;
    match bm {
        Efi(SecureBoot::Enabled) => BootModeInformation {
            ...
            mode: BootMode::Efi,
            secureboot: true,
        },
        Efi(SecureBoot::Disabled) => BootModeInformation {
            ...
            mode: BootMode::Efi,
            secureboot: false,
        },
        Bios => BootModeInformation {
            ...
            mode: BootMode::LegacyBios,
            secureboot: false,
        },
    }

IMO that's easier to follow.

> +    match bm {
> +        boot_mode::BootModeInformation::Efi(secureboot) => match secureboot {
> +            boot_mode::SecureBoot::Enabled => BootModeInformation {
> +                mode: pbs_api_types::BootMode::Efi,
> +                secureboot: true,
> +            },
> +            boot_mode::SecureBoot::Disabled => BootModeInformation {
> +                mode: pbs_api_types::BootMode::Efi,
> +                secureboot: false,
> +            },
> +        },
> +        boot_mode::BootModeInformation::Bios => BootModeInformation {
> +            mode: pbs_api_types::BootMode::LegacyBios,
> +            secureboot: false,
> +        },
> +    }
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -79,6 +100,8 @@ async fn get_status(
>  
>      let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
>  
> +    let boot_info = boot_mode_to_info(boot_mode::boot_mode());
> +
>      Ok(NodeStatus {
>          memory,
>          swap,
> @@ -96,6 +119,7 @@ async fn get_status(
>          info: NodeInformation {
>              fingerprint: crate::cert_info()?.fingerprint()?,
>          },
> +        boot_info,
>      })
>  }
>  
> diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
> index 2551c9a5..cba6d2a1 100644
> --- a/www/panel/NodeInfo.js
> +++ b/www/panel/NodeInfo.js

^ ui patches should be separate

> @@ -147,6 +147,21 @@ Ext.define('PBS.NodeInfoPanel', {
>  	    textField: 'kversion',
>  	    value: '',
>  	},
> +	{
> +	    colspan: 2,
> +	    title: gettext('Boot Mode'),
> +	    printBar: false,
> +	    textField: 'boot-info',
> +	    renderer: boot => {
> +		if (boot.mode === 'legacy-bios') {
> +		    return 'Legacy BIOS';
> +		} else if (boot.mode === 'efi') {
> +		    return `EFI${boot.secureboot ? ' (Secure Boot)' : ''}`;
> +		}
> +		return Proxmox.Utils.unknownText;
> +	    },
> +	    value: '',
> +	},
>  	{
>  	    xtype: 'pmxNodeInfoRepoStatus',
>  	    itemId: 'repositoryStatus',
> -- 
> 2.39.2

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

Thanks for the review!

On 11/27/23 14:10, Wolfgang Bumiller wrote:
> On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
>> +
>> +#[api]
>> +#[derive(Serialize, Deserialize, Default)]
> And Clone + Copy
Agree
>> +#[serde(rename_all = "kebab-case")]
>> +/// The possible BootModes
>> +pub enum BootMode {
>> +    /// The BootMode is EFI/UEFI
>> +    Efi,
>> +    /// The BootMode is Legacy BIOS
>> +    #[default]
> ^ do we *need* Default on this type? And why is Bios the default?
Removed it. Was enabled on the `NodeStatus` struct and cascaded down, 
but afaik we can remove it
on the `NodeStatus` struct as well and get rid of it.
>
>> +    LegacyBios,
>> +}
>> +
>> +#[api]
>> +#[derive(Serialize, Deserialize, Default)]
> And Clone
yep
>> +fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> BootModeInformation {
> I'd prefer the match to be a single level by doing
>
>      use boot_mode::{BootModeIntofmation::*, SecureBoot};
>      use pbs_api_types::BootMode;
>      match bm {
>          Efi(SecureBoot::Enabled) => BootModeInformation {
>              ...
>              mode: BootMode::Efi,
>              secureboot: true,
>          },
>          Efi(SecureBoot::Disabled) => BootModeInformation {
>              ...
>              mode: BootMode::Efi,
>              secureboot: false,
>          },
>          Bios => BootModeInformation {
>              ...
>              mode: BootMode::LegacyBios,
>              secureboot: false,
>          },
>      }
>
> IMO that's easier to follow.
Good point!

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Wolfgang Bumiller]

On Mon, Nov 27, 2023 at 02:28:14PM +0100, Gabriel Goller wrote:
> Thanks for the review!
> 
> On 11/27/23 14:10, Wolfgang Bumiller wrote:
> > On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
> > > +
> > > +#[api]
> > > +#[derive(Serialize, Deserialize, Default)]
> > And Clone + Copy
> Agree
> > > +#[serde(rename_all = "kebab-case")]
> > > +/// The possible BootModes
> > > +pub enum BootMode {
> > > +    /// The BootMode is EFI/UEFI
> > > +    Efi,
> > > +    /// The BootMode is Legacy BIOS
> > > +    #[default]
> > ^ do we *need* Default on this type? And why is Bios the default?
> Removed it. Was enabled on the `NodeStatus` struct and cascaded down, but
> afaik we can remove it
> on the `NodeStatus` struct as well and get rid of it.

IMO this is one of those options where we can't have a default, so if a
struct containing it needs to be Default, this value should be an
Option<> in there instead.

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

On 11/27/23 14:53, Wolfgang Bumiller wrote:
> On Mon, Nov 27, 2023 at 02:28:14PM +0100, Gabriel Goller wrote:
>> Thanks for the review!
>>
>> On 11/27/23 14:10, Wolfgang Bumiller wrote:
>>> On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
>>>> +
>>>> +#[api]
>>>> +#[derive(Serialize, Deserialize, Default)]
>>> And Clone + Copy
>> Agree
>>>> +#[serde(rename_all = "kebab-case")]
>>>> +/// The possible BootModes
>>>> +pub enum BootMode {
>>>> +    /// The BootMode is EFI/UEFI
>>>> +    Efi,
>>>> +    /// The BootMode is Legacy BIOS
>>>> +    #[default]
>>> ^ do we *need* Default on this type? And why is Bios the default?
>> Removed it. Was enabled on the `NodeStatus` struct and cascaded down, but
>> afaik we can remove it
>> on the `NodeStatus` struct as well and get rid of it.
> IMO this is one of those options where we can't have a default, so if a
> struct containing it needs to be Default, this value should be an
> Option<> in there instead.
Agree.

But what do you think about the SecureBoot enum in the proxmox_sys crate?
Currently I have this:

#[derive(Clone, Copy)]
pub enum SecureBoot {
     /// SecureBoot is enabled
     Enabled,
     /// SecureBoot is disabled
     Disabled,
}
impl SecureBoot {
     pub fn query() -> SecureBoot {
         lazy_static::lazy_static!(
             static ref SECURE_BOOT: Mutex<Option<SecureBoot>> = 
Mutex::new(None);
         );

         let mut last = SECURE_BOOT.lock().unwrap();
         let value = last.or_else(|| {
             // Check if SecureBoot is enabled
             // Attention: this file is not seekable!
             // Spec: 
https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html?highlight=8be4d#globally-defined-variables
             let efivar = std::fs::File::open(
"/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
             );
             if let Ok(mut file) = efivar {
                 let mut buf = [0; 5];
                 let Ok(_) = file.read_exact(&mut buf) else {
                         return Some(SecureBoot::Disabled);
                     };
                 if buf[4] == 1 {
                     Some(SecureBoot::Enabled)
                 } else {
                     Some(SecureBoot::Disabled)
                 }
             } else {
                 Some(SecureBoot::Disabled)
             }
         });
         *last = value;
         value.unwrap()
     }
}

Although we could make the function return a bool (then we'd have a 
free-standing function again), which would be simpler... (+ we convert 
it in pbs to a bool anyway)
One advantage of my approach is that we are more flexible, could add 
another option, rename them, etc...

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Wolfgang Bumiller]

On Mon, Nov 27, 2023 at 03:02:18PM +0100, Gabriel Goller wrote:
> 
> On 11/27/23 14:53, Wolfgang Bumiller wrote:
> > On Mon, Nov 27, 2023 at 02:28:14PM +0100, Gabriel Goller wrote:
> > > Thanks for the review!
> > > 
> > > On 11/27/23 14:10, Wolfgang Bumiller wrote:
> > > > On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
> > > > > +
> > > > > +#[api]
> > > > > +#[derive(Serialize, Deserialize, Default)]
> > > > And Clone + Copy
> > > Agree
> > > > > +#[serde(rename_all = "kebab-case")]
> > > > > +/// The possible BootModes
> > > > > +pub enum BootMode {
> > > > > +    /// The BootMode is EFI/UEFI
> > > > > +    Efi,
> > > > > +    /// The BootMode is Legacy BIOS
> > > > > +    #[default]
> > > > ^ do we *need* Default on this type? And why is Bios the default?
> > > Removed it. Was enabled on the `NodeStatus` struct and cascaded down, but
> > > afaik we can remove it
> > > on the `NodeStatus` struct as well and get rid of it.
> > IMO this is one of those options where we can't have a default, so if a
> > struct containing it needs to be Default, this value should be an
> > Option<> in there instead.
> Agree.
> 
> But what do you think about the SecureBoot enum in the proxmox_sys crate?
> Currently I have this:
> 
> #[derive(Clone, Copy)]
> pub enum SecureBoot {
>     /// SecureBoot is enabled
>     Enabled,
>     /// SecureBoot is disabled
>     Disabled,
> }
> impl SecureBoot {
>     pub fn query() -> SecureBoot {
>         lazy_static::lazy_static!(
>             static ref SECURE_BOOT: Mutex<Option<SecureBoot>> =
> Mutex::new(None);
>         );
> 
>         let mut last = SECURE_BOOT.lock().unwrap();
>         let value = last.or_else(|| {
>             // Check if SecureBoot is enabled
>             // Attention: this file is not seekable!
>             // Spec: https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html?highlight=8be4d#globally-defined-variables
>             let efivar = std::fs::File::open(
> "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
>             );
>             if let Ok(mut file) = efivar {
>                 let mut buf = [0; 5];
>                 let Ok(_) = file.read_exact(&mut buf) else {
>                         return Some(SecureBoot::Disabled);
>                     };
>                 if buf[4] == 1 {
>                     Some(SecureBoot::Enabled)
>                 } else {
>                     Some(SecureBoot::Disabled)
>                 }
>             } else {
>                 Some(SecureBoot::Disabled)
>             }
>         });
>         *last = value;
>         value.unwrap()
>     }
> }
> 
> Although we could make the function return a bool (then we'd have a
> free-standing function again), which would be simpler... (+ we convert it in
> pbs to a bool anyway)
> One advantage of my approach is that we are more flexible, could add another
> option, rename them, etc...

Sorry for the late reply.
IMO both are fine. After all, if we need to change away from a bool we
can just mark the function as #[deprecated] and move on from there with
compiler help.
I don't think we'd really lose any flexibility if in the end we turn it
into a boolean on the API facing side anyway, as a change there would be
an API break after all, while an internal change does not matter that
much.

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

Submitted a new version!

On 11/29/23 09:58, Wolfgang Bumiller wrote:
> On Mon, Nov 27, 2023 at 03:02:18PM +0100, Gabriel Goller wrote:
>> On 11/27/23 14:53, Wolfgang Bumiller wrote:
>>> On Mon, Nov 27, 2023 at 02:28:14PM +0100, Gabriel Goller wrote:
>>>> Thanks for the review!
>>>>
>>>> On 11/27/23 14:10, Wolfgang Bumiller wrote:
>>>>> On Mon, Nov 27, 2023 at 11:52:37AM +0100, Gabriel Goller wrote:
>>>>>> +
>>>>>> +#[api]
>>>>>> +#[derive(Serialize, Deserialize, Default)]
>>>>> And Clone + Copy
>>>> Agree
>>>>>> +#[serde(rename_all = "kebab-case")]
>>>>>> +/// The possible BootModes
>>>>>> +pub enum BootMode {
>>>>>> +    /// The BootMode is EFI/UEFI
>>>>>> +    Efi,
>>>>>> +    /// The BootMode is Legacy BIOS
>>>>>> +    #[default]
>>>>> ^ do we *need* Default on this type? And why is Bios the default?
>>>> Removed it. Was enabled on the `NodeStatus` struct and cascaded down, but
>>>> afaik we can remove it
>>>> on the `NodeStatus` struct as well and get rid of it.
>>> IMO this is one of those options where we can't have a default, so if a
>>> struct containing it needs to be Default, this value should be an
>>> Option<> in there instead.
>> Agree.
>>
>> But what do you think about the SecureBoot enum in the proxmox_sys crate?
>> Currently I have this:
>>
>> #[derive(Clone, Copy)]
>> pub enum SecureBoot {
>>      /// SecureBoot is enabled
>>      Enabled,
>>      /// SecureBoot is disabled
>>      Disabled,
>> }
>> impl SecureBoot {
>>      pub fn query() -> SecureBoot {
>>          lazy_static::lazy_static!(
>>              static ref SECURE_BOOT: Mutex<Option<SecureBoot>> =
>> Mutex::new(None);
>>          );
>>
>>          let mut last = SECURE_BOOT.lock().unwrap();
>>          let value = last.or_else(|| {
>>              // Check if SecureBoot is enabled
>>              // Attention: this file is not seekable!
>>              // Spec: https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html?highlight=8be4d#globally-defined-variables
>>              let efivar = std::fs::File::open(
>> "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
>>              );
>>              if let Ok(mut file) = efivar {
>>                  let mut buf = [0; 5];
>>                  let Ok(_) = file.read_exact(&mut buf) else {
>>                          return Some(SecureBoot::Disabled);
>>                      };
>>                  if buf[4] == 1 {
>>                      Some(SecureBoot::Enabled)
>>                  } else {
>>                      Some(SecureBoot::Disabled)
>>                  }
>>              } else {
>>                  Some(SecureBoot::Disabled)
>>              }
>>          });
>>          *last = value;
>>          value.unwrap()
>>      }
>> }
>>
>> Although we could make the function return a bool (then we'd have a
>> free-standing function again), which would be simpler... (+ we convert it in
>> pbs to a bool anyway)
>> One advantage of my approach is that we are more flexible, could add another
>> option, rename them, etc...
> Sorry for the late reply.
> IMO both are fine. After all, if we need to change away from a bool we
> can just mark the function as #[deprecated] and move on from there with
> compiler help.
> I don't think we'd really lose any flexibility if in the end we turn it
> into a boolean on the API facing side anyway, as a change there would be
> an API break after all, while an internal change does not matter that
> much.

[pbs-devel] [PATCH v4 proxmox-backup 3/3] node: status: declutter kernel-version

[Gabriel Goller]

Return a struct with all the components of the kernel version like it
has been done in pve. Extract and display the build version and kernel
release nicely.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-api-types/src/node.rs | 47 ++++++++++++++++++++++++++++++++++++++-
 src/api2/node/status.rs   | 18 +++++++--------
 www/panel/NodeInfo.js     | 13 +++++++++--
 3 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
index 8bcf8fd6..6004acb3 100644
--- a/pbs-api-types/src/node.rs
+++ b/pbs-api-types/src/node.rs
@@ -1,3 +1,5 @@
+use std::ffi::OsStr;
+
 use proxmox_schema::*;
 use serde::{Deserialize, Serialize};
 
@@ -38,6 +40,47 @@ pub struct NodeInformation {
     pub fingerprint: String,
 }
 
+#[api]
+#[derive(Serialize, Deserialize, Default)]
+#[serde(rename_all = "lowercase")]
+/// The current kernel version (output of `uname`)
+pub struct KernelVersionInformation {
+    /// The systemname/nodename
+    pub sysname: String,
+    /// The kernel release number
+    pub release: String,
+    /// The kernel version
+    pub version: String,
+    /// The machine architecture
+    pub machine: String,
+}
+
+impl KernelVersionInformation {
+    pub fn from_ostr(sysname: &OsStr, release: &OsStr, version: &OsStr, machine: &OsStr) -> Self {
+        KernelVersionInformation {
+            sysname: sysname
+                .to_os_string()
+                .into_string()
+                .unwrap_or("".to_string()),
+            release: release
+                .to_os_string()
+                .into_string()
+                .unwrap_or("".to_string()),
+            version: version
+                .to_os_string()
+                .into_string()
+                .unwrap_or("".to_string()),
+            machine: machine
+                .to_os_string()
+                .into_string()
+                .unwrap_or("".to_string()),
+        }
+    }
+
+    pub fn get_legacy(&self) -> String {
+        format!("{} {} {}", self.sysname, self.release, self.version)
+    }
+}
 
 #[api]
 #[derive(Serialize, Deserialize, Default)]
@@ -112,7 +155,9 @@ pub struct NodeStatus {
     pub uptime: u64,
     /// Load for 1, 5 and 15 minutes.
     pub loadavg: [f64; 3],
-    /// The current kernel version.
+    /// The current kernel version (NEW struct type).
+    pub current_kernel: KernelVersionInformation,
+    /// The current kernel version (LEGACY string type).
     pub kversion: String,
     /// Total CPU usage since last query.
     pub cpu: f64,
diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
index 21f1bc2c..e4723938 100644
--- a/src/api2/node/status.rs
+++ b/src/api2/node/status.rs
@@ -1,4 +1,3 @@
-use std::os::unix::ffi::OsStrExt;
 use std::process::Command;
 
 use anyhow::{bail, format_err, Error};
@@ -11,8 +10,8 @@ use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    BootModeInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT,
-    PRIV_SYS_POWER_MANAGEMENT,
+    BootModeInformation, KernelVersionInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA,
+    PRIV_SYS_AUDIT, PRIV_SYS_POWER_MANAGEMENT,
 };
 
 use pbs_api_types::{
@@ -91,11 +90,11 @@ async fn get_status(
     let cpuinfo = procfs_to_node_cpu_info(cpuinfo);
 
     let uname = nix::sys::utsname::uname()?;
-    let kversion = format!(
-        "{} {} {}",
-        std::str::from_utf8(uname.sysname().as_bytes())?,
-        std::str::from_utf8(uname.release().as_bytes())?,
-        std::str::from_utf8(uname.version().as_bytes())?
+    let kernel_version = KernelVersionInformation::from_ostr(
+        uname.sysname(),
+        uname.release(),
+        uname.version(),
+        uname.machine(),
     );
 
     let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
@@ -112,7 +111,8 @@ async fn get_status(
         },
         uptime: procfs::read_proc_uptime()?.0 as u64,
         loadavg,
-        kversion,
+        kversion: kernel_version.get_legacy(),
+        current_kernel: kernel_version,
         cpuinfo,
         cpu,
         wait,
diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
index cba6d2a1..72f97c7c 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -140,11 +140,20 @@ Ext.define('PBS.NodeInfoPanel', {
 	    value: '',
 	},
 	{
-	    itemId: 'kversion',
 	    colspan: 2,
 	    title: gettext('Kernel Version'),
 	    printBar: false,
-	    textField: 'kversion',
+	    // TODO: remove with next major and only use newish current-kernel textfield
+	    multiField: true,
+	    //textField: 'current-kernel',
+	    renderer: ({ data }) => {
+		if (!data['current-kernel']) {
+		    return data.kversion;
+		}
+		let kernel = data['current-kernel'];
+		let buildDate = kernel.version.match(/\((.+)\)\s*$/)[1] ?? 'unknown';
+		return `${kernel.sysname} ${kernel.release} (${buildDate})`;
+	    },
 	    value: '',
 	},
 	{
-- 
2.39.2