[pbs-devel] [PATCH proxmox-backup 0/2] Add bootmode, improve kernel version

[pbs-devel] [PATCH proxmox-backup 0/2] Add bootmode, 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.


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

 pbs-api-types/src/node.rs | 36 +++++++++++++++++++++++++++++++---
 src/api2/node/status.rs   | 41 +++++++++++++++++++++++++++++++--------
 www/panel/NodeInfo.js     | 21 ++++++++++++++++++++
 3 files changed, 87 insertions(+), 11 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 1/2] 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 | 20 ++++++++++++++++++--
 src/api2/node/status.rs   | 28 +++++++++++++++++++++++++++-
 www/panel/NodeInfo.js     | 17 +++++++++++++++++
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
index 704215bb..6d1fa7f0 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,21 @@ pub struct NodeInformation {
     pub fingerprint: String,
 }
 
+
+#[api]
+#[derive(Serialize, Deserialize, Default)]
+#[serde(rename_all = "lowercase")]
+/// The possible BootModes
+pub enum BootModeInformation {
+    /// The BootMode is EFI/UEFI
+    Efi,
+    /// The BootMode is EFI/UEFI with Secure Boot enabled
+    EfiSecureBoot,
+    /// The BootMode is Legacy BIOS
+    #[default]
+    Bios,
+}
+
 #[api]
 #[derive(Serialize, Deserialize, Default)]
 #[serde(rename_all = "kebab-case")]
@@ -97,4 +111,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..1b3b9e33 100644
--- a/src/api2/node/status.rs
+++ b/src/api2/node/status.rs
@@ -1,4 +1,7 @@
+use std::fs::File;
+use std::io::Read;
 use std::os::unix::prelude::OsStrExt;
+use std::path::Path;
 use std::process::Command;
 
 use anyhow::{bail, format_err, Error};
@@ -10,7 +13,8 @@ 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::{
@@ -79,6 +83,27 @@ async fn get_status(
 
     let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
 
+    let boot_info: BootModeInformation;
+    if Path::new("/sys/firmware/efi").exists() {
+        // Check if SecureBoot is enabled
+        // Attention: this file is not seekable!
+        let efivar =
+            File::open("/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c");
+        if let Ok(mut file) = efivar {
+            let mut buf = [0; 5];
+            file.read_exact(&mut buf)?;
+            if buf[4..] == [1] {
+                boot_info = BootModeInformation::EfiSecureBoot;
+            } else {
+                boot_info = BootModeInformation::Efi;
+            }
+        } else {
+            boot_info = BootModeInformation::Efi;
+        }
+    } else {
+        boot_info = BootModeInformation::Bios;
+    }
+
     Ok(NodeStatus {
         memory,
         swap,
@@ -96,6 +121,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..14f84a2e 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -147,6 +147,23 @@ Ext.define('PBS.NodeInfoPanel', {
 	    textField: 'kversion',
 	    value: '',
 	},
+	{
+	    colspan: 2,
+	    title: gettext('Boot Mode'),
+	    printBar: false,
+	    textField: 'boot-info',
+	    renderer: boot_mode => {
+	        if (boot_mode === 'bios') {
+	            return 'Legacy BIOS';
+	        } else if (boot_mode === 'efi') {
+	            return 'EFI';
+	        } else if (boot_mode === 'efisecureboot') {
+	            return 'EFI (Secure Boot)';
+	        }
+	        return Proxmox.Utils.unknownText;
+	    },
+	    value: '',
+	},
 	{
 	    xtype: 'pmxNodeInfoRepoStatus',
 	    itemId: 'repositoryStatus',
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 2/2] 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 | 16 +++++++++++++++-
 src/api2/node/status.rs   | 17 ++++++++---------
 www/panel/NodeInfo.js     |  4 ++++
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
index 6d1fa7f0..5963658a 100644
--- a/pbs-api-types/src/node.rs
+++ b/pbs-api-types/src/node.rs
@@ -38,6 +38,20 @@ 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,
+}
 
 #[api]
 #[derive(Serialize, Deserialize, Default)]
@@ -104,7 +118,7 @@ pub struct NodeStatus {
     /// Load for 1, 5 and 15 minutes.
     pub loadavg: [f64; 3],
     /// The current kernel version.
-    pub kversion: String,
+    pub kversion: KernelVersionInformation,
     /// Total CPU usage since last query.
     pub cpu: f64,
     /// Total IO wait since last query.
diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
index 1b3b9e33..fae2b49b 100644
--- a/src/api2/node/status.rs
+++ b/src/api2/node/status.rs
@@ -1,6 +1,5 @@
 use std::fs::File;
 use std::io::Read;
-use std::os::unix::prelude::OsStrExt;
 use std::path::Path;
 use std::process::Command;
 
@@ -13,8 +12,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::{
@@ -74,12 +73,12 @@ 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 kversion = KernelVersionInformation {
+        sysname: uname.sysname().to_os_string().into_string().unwrap(),
+        release: uname.release().to_os_string().into_string().unwrap(),
+        version: uname.version().to_os_string().into_string().unwrap(),
+        machine: uname.machine().to_os_string().into_string().unwrap(),
+    };
 
     let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
 
diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
index 14f84a2e..25927ebc 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -145,6 +145,10 @@ Ext.define('PBS.NodeInfoPanel', {
 	    title: gettext('Kernel Version'),
 	    printBar: false,
 	    textField: 'kversion',
+	    renderer: (kernel) => {
+	        let buildDate = kernel.version.match(/\((.+)\)\s*$/)[1] ?? 'unknown';
+	        return `${kernel.sysname} ${kernel.release} (${buildDate})`;
+	    },
 	    value: '',
 	},
 	{
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup 2/2] node: status: declutter kernel-version

[Dietmar Maurer]

> +    let kversion = KernelVersionInformation {
> +        sysname: uname.sysname().to_os_string().into_string().unwrap(),
> +        release: uname.release().to_os_string().into_string().unwrap(),
> +        version: uname.version().to_os_string().into_string().unwrap(),
> +        machine: uname.machine().to_os_string().into_string().unwrap(),
> +    };

Is it really safe to use unwrap here? If so, I normally add a short
comment to the code to explain why.

Re: [pbs-devel] [PATCH proxmox-backup 2/2] node: status: declutter kernel-version

[Thomas Lamprecht]

Am 23/11/2023 um 14:09 schrieb 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.
> 

Proxmox VE has added a new property though to keep backward compat with
our API consumer?!

Re: [pbs-devel] [PATCH proxmox-backup 2/2] node: status: declutter kernel-version

[Thomas Lamprecht]

Am 23/11/2023 um 17:15 schrieb Dietmar Maurer:
>> +    let kversion = KernelVersionInformation {
>> +        sysname: uname.sysname().to_os_string().into_string().unwrap(),
>> +        release: uname.release().to_os_string().into_string().unwrap(),
>> +        version: uname.version().to_os_string().into_string().unwrap(),
>> +        machine: uname.machine().to_os_string().into_string().unwrap(),
>> +    };
> 
> Is it really safe to use unwrap here? If so, I normally add a short
> comment to the code to explain why.
> 

Yeah, I *really* dislike .unwrap() calls, especially if in common code,
i.e., not isolated into it's own function with a clear comment that
states why this never can fail, and IMO it can fail because those strings
are just bytes for the kernel and can be set on boot, so it might be
possible to set them to something that isn't valid unicode..

So, please use unwarp_or and maybe it would be better to move this to
an impl on the KernelVersionInformation struct, that could also host
a to_legacy() printing the old unwieldy string, as I'd not just switch
the old variable from string to object, that's an api break.

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

[Thomas Lamprecht]

Am 23/11/2023 um 14:09 schrieb Gabriel Goller:
> @@ -79,6 +83,27 @@ async fn get_status(
>  
>      let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
>  
> +    let boot_info: BootModeInformation;
> +    if Path::new("/sys/firmware/efi").exists() {
> +        // Check if SecureBoot is enabled
> +        // Attention: this file is not seekable!
> +        let efivar =
> +            File::open("/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c");
> +        if let Ok(mut file) = efivar {
> +            let mut buf = [0; 5];
> +            file.read_exact(&mut buf)?;
> +            if buf[4..] == [1] {
> +                boot_info = BootModeInformation::EfiSecureBoot;
> +            } else {
> +                boot_info = BootModeInformation::Efi;
> +            }
> +        } else {
> +            boot_info = BootModeInformation::Efi;
> +        }
> +    } else {
> +        boot_info = BootModeInformation::Bios;
> +    }

please move above into it's own helper method, it could even live in
some more common module (but we can still move it later, so no hard
feelings on that).

And I'd also like if this caches the boot mode, e.g. in a LazyStatic,
similar to how Proxmox VE does it, as I know that lots of firmware is
rather a mess, so constantly reading EFI vars might make trigger some
bugs (or just hang suddenly).

> +
>      Ok(NodeStatus {
>          memory,
>          swap,
> @@ -96,6 +121,7 @@ async fn get_status(

Re: [pbs-devel] [PATCH proxmox-backup 2/2] node: status: declutter kernel-version

[Thomas Lamprecht]

Am 23/11/2023 um 17:29 schrieb Thomas Lamprecht:
> and IMO it can fail because those strings are just bytes for the kernel and
> can be set on boot, so it might be

"can be set on build" is what I wanted to write..

> possible to set them to something that isn't valid unicode..

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

[Gabriel Goller]

Thanks for the review!

On 11/23/23 17:32, Thomas Lamprecht wrote:
> Am 23/11/2023 um 14:09 schrieb Gabriel Goller:
>> @@ -79,6 +83,27 @@ async fn get_status(
>>   
>>       let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
>>   
>> +    let boot_info: BootModeInformation;
>> +    if Path::new("/sys/firmware/efi").exists() {
>> +        // Check if SecureBoot is enabled
>> +        // Attention: this file is not seekable!
>> +        let efivar =
>> +            File::open("/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c");
>> +        if let Ok(mut file) = efivar {
>> +            let mut buf = [0; 5];
>> +            file.read_exact(&mut buf)?;
>> +            if buf[4..] == [1] {
>> +                boot_info = BootModeInformation::EfiSecureBoot;
>> +            } else {
>> +                boot_info = BootModeInformation::Efi;
>> +            }
>> +        } else {
>> +            boot_info = BootModeInformation::Efi;
>> +        }
>> +    } else {
>> +        boot_info = BootModeInformation::Bios;
>> +    }
> please move above into it's own helper method, it could even live in
> some more common module (but we can still move it later, so no hard
> feelings on that).
How about we move it to proxmox-sys?
> And I'd also like if this caches the boot mode, e.g. in a LazyStatic,
> similar to how Proxmox VE does it, as I know that lots of firmware is
> rather a mess, so constantly reading EFI vars might make trigger some
> bugs (or just hang suddenly).
Yep, I would also put the LazyStatic in `proxmox-sys`.
>> +
>>       Ok(NodeStatus {
>>           memory,
>>           swap,
>> @@ -96,6 +121,7 @@ async fn get_status(

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

[Thomas Lamprecht]

Am 24/11/2023 um 10:04 schrieb Gabriel Goller:
>> please move above into it's own helper method, it could even live in
>> some more common module (but we can still move it later, so no hard
>> feelings on that).
> How about we move it to proxmox-sys?

That crate is already to huge and is in serious needs of getting split
up as it blows up compile times for smaller projects that need only a
few bits of it, so that's why I avoided the suggestion.

But not to hard feelings for now, it'd won't get that much worse and
having it there might allow for a better decision about how to split
it, just don't shove it into some random overly general module.

Re: [pbs-devel] [PATCH proxmox-backup 2/2] node: status: declutter kernel-version

[Gabriel Goller]

On 11/23/23 17:29, Thomas Lamprecht wrote:

> Am 23/11/2023 um 17:15 schrieb Dietmar Maurer:
>>> +    let kversion = KernelVersionInformation {
>>> +        sysname: uname.sysname().to_os_string().into_string().unwrap(),
>>> +        release: uname.release().to_os_string().into_string().unwrap(),
>>> +        version: uname.version().to_os_string().into_string().unwrap(),
>>> +        machine: uname.machine().to_os_string().into_string().unwrap(),
>>> +    };
>> Is it really safe to use unwrap here? If so, I normally add a short
>> comment to the code to explain why.
>>
> Yeah, I *really* dislike .unwrap() calls, especially if in common code,
> i.e., not isolated into it's own function with a clear comment that
> states why this never can fail, and IMO it can fail because those strings
> are just bytes for the kernel and can be set on boot, so it might be
> possible to set them to something that isn't valid unicode..
>
> So, please use unwarp_or and maybe it would be better to move this to
> an impl on the KernelVersionInformation struct, that could also host
> a to_legacy() printing the old unwieldy string, as I'd not just switch
> the old variable from string to object, that's an api break.
Agree, I will use a `unwrap_or` and return an empty string.
Didn't know backwards compat was a thing here, so I will also return the
old property with the legacy string :)

Re: [pbs-devel] [PATCH proxmox-backup 0/2] Add bootmode, improve kernel version

[Gabriel Goller]

Submitted a v2!

On 11/23/23 14:09, Gabriel Goller wrote:
> 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.
>
>
> Gabriel Goller (2):
>    node: status: added bootmode
>    node: status: declutter kernel-version
>
>   pbs-api-types/src/node.rs | 36 +++++++++++++++++++++++++++++++---
>   src/api2/node/status.rs   | 41 +++++++++++++++++++++++++++++++--------
>   www/panel/NodeInfo.js     | 21 ++++++++++++++++++++
>   3 files changed, 87 insertions(+), 11 deletions(-)
>