[pbs-devel] [PATCH proxmox-backup 0/2] Fix DatastoreListSummary on limited permissions

[pbs-devel] [PATCH proxmox-backup 0/2] Fix DatastoreListSummary on limited permissions

[Gabriel Goller]

When the user has limited permissions on a datastore (e.g `NoAccess` on the whole 
datastore and `DatastoreBackup` on a namespace inside the datastore) the Datastore
List shows weird values. 

Instead of returning -1 we return an Option and handle missing values on the
frontend.



proxmox-backup:

Gabriel Goller (2):
  ui: datastore summary handle non-existent values
  status: use Option on avail/used datastore attrs

 pbs-api-types/src/datastore.rs        | 19 +++++++++++--------
 src/api2/status.rs                    |  6 +++---
 www/datastore/DataStoreListSummary.js | 20 ++++++++++++++------
 3 files changed, 28 insertions(+), 17 deletions(-)


Summary over all repositories:
  3 files changed, 28 insertions(+), 17 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH proxmox-backup 1/2] ui: datastore summary handle non-existent values

[Gabriel Goller]

Correctly display missing 'avail' and 'used' attributes in the
datatstore summary. This simply sets it to 0, so that we don't get any
errors in the console.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/datastore/DataStoreListSummary.js | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index 968239b0..b908034d 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -52,12 +52,20 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
 	    vm.set('maintenance', '');
 	}
 
-	let total = statusData.avail + statusData.used;
-	let usage = statusData.used / total;
-	let usagetext = Ext.String.format(gettext('{0} of {1}'),
-	    Proxmox.Utils.format_size(statusData.used, true),
-	    Proxmox.Utils.format_size(total, true),
-	);
+	let usagetext;
+	let usage;
+
+	if (Object.hasOwn(statusData, 'avail') && Object.hasOwn(statusData, 'used')) {
+	    let total = statusData.avail + statusData.used;
+	    usage = statusData.used / total;
+	    usagetext = Ext.String.format(gettext('{0} of {1}'),
+		Proxmox.Utils.format_size(statusData.used, true),
+		Proxmox.Utils.format_size(total, true),
+	    );
+	} else {
+	    usagetext = Ext.String.format(gettext('{0} of {1}'), 0, 0);
+	    usage = 0;
+	}
 
 	let usagePanel = me.lookup('usage');
 	usagePanel.updateValue(usage, usagetext);
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Gabriel Goller]

Instead of returning -1 if we can't get the attributes, we use an
Option which will not be serialized on `None`.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 19 +++++++++++--------
 src/api2/status.rs             |  6 +++---
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index d4ead1d1..c361dc30 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1302,12 +1302,15 @@ pub struct DataStoreStatus {
 /// Status of a Datastore
 pub struct DataStoreStatusListItem {
     pub store: String,
-    /// The Size of the underlying storage in bytes. (-1 on error)
-    pub total: i64,
-    /// The used bytes of the underlying storage. (-1 on error)
-    pub used: i64,
+    /// The Size of the underlying storage in bytes.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub total: Option<i64>,
+    /// The used bytes of the underlying storage.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub used: Option<i64>,
     /// The available bytes of the underlying storage. (-1 on error)
-    pub avail: i64,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub avail: Option<i64>,
     /// A list of usages of the past (last Month).
     #[serde(skip_serializing_if = "Option::is_none")]
     pub history: Option<Vec<Option<f64>>>,
@@ -1335,9 +1338,9 @@ impl DataStoreStatusListItem {
     pub fn empty(store: &str, err: Option<String>) -> Self {
         DataStoreStatusListItem {
             store: store.to_owned(),
-            total: -1,
-            used: -1,
-            avail: -1,
+            total: None,
+            used: None,
+            avail: None,
             history: None,
             history_start: None,
             history_delta: None,
diff --git a/src/api2/status.rs b/src/api2/status.rs
index ff7d4cbd..6e758187 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -68,9 +68,9 @@ pub async fn datastore_status(
 
         let mut entry = DataStoreStatusListItem {
             store: store.clone(),
-            total: status.total as i64,
-            used: status.used as i64,
-            avail: status.available as i64,
+            total: Some(status.total as i64),
+            used: Some(status.used as i64),
+            avail: Some(status.available as i64),
             history: None,
             history_start: None,
             history_delta: None,
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Dietmar Maurer]

But this is not backwards compatible. Deserialization fail now for -1 (when connecting to an old server which returns -1)?

> On 7.12.2023 15:35 CET Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> Instead of returning -1 if we can't get the attributes, we use an
> Option which will not be serialized on `None`.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 19 +++++++++++--------
>  src/api2/status.rs             |  6 +++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index d4ead1d1..c361dc30 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1302,12 +1302,15 @@ pub struct DataStoreStatus {
>  /// Status of a Datastore
>  pub struct DataStoreStatusListItem {
>      pub store: String,
> -    /// The Size of the underlying storage in bytes. (-1 on error)
> -    pub total: i64,
> -    /// The used bytes of the underlying storage. (-1 on error)
> -    pub used: i64,
> +    /// The Size of the underlying storage in bytes.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub total: Option<i64>,
> +    /// The used bytes of the underlying storage.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub used: Option<i64>,
>      /// The available bytes of the underlying storage. (-1 on error)
> -    pub avail: i64,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub avail: Option<i64>,
>      /// A list of usages of the past (last Month).
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub history: Option<Vec<Option<f64>>>,
> @@ -1335,9 +1338,9 @@ impl DataStoreStatusListItem {
>      pub fn empty(store: &str, err: Option<String>) -> Self {
>          DataStoreStatusListItem {
>              store: store.to_owned(),
> -            total: -1,
> -            used: -1,
> -            avail: -1,
> +            total: None,
> +            used: None,
> +            avail: None,
>              history: None,
>              history_start: None,
>              history_delta: None,
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index ff7d4cbd..6e758187 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -68,9 +68,9 @@ pub async fn datastore_status(
>  
>          let mut entry = DataStoreStatusListItem {
>              store: store.clone(),
> -            total: status.total as i64,
> -            used: status.used as i64,
> -            avail: status.available as i64,
> +            total: Some(status.total as i64),
> +            used: Some(status.used as i64),
> +            avail: Some(status.available as i64),
>              history: None,
>              history_start: None,
>              history_delta: None,
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Dietmar Maurer]

Oh, Ignore me - I forgot that we do not have any GUI that uses rust
for deserialization (it is only my prototype)...

> On 7.12.2023 18:07 CET Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> But this is not backwards compatible. Deserialization fail now for -1 (when connecting to an old server which returns -1)?

Re: [pbs-devel] [PATCH proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Dietmar Maurer]

But why is it still i64? I would expect u64.


> On 7.12.2023 15:35 CET Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> Instead of returning -1 if we can't get the attributes, we use an
> Option which will not be serialized on `None`.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 19 +++++++++++--------
>  src/api2/status.rs             |  6 +++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index d4ead1d1..c361dc30 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1302,12 +1302,15 @@ pub struct DataStoreStatus {
>  /// Status of a Datastore
>  pub struct DataStoreStatusListItem {
>      pub store: String,
> -    /// The Size of the underlying storage in bytes. (-1 on error)
> -    pub total: i64,
> -    /// The used bytes of the underlying storage. (-1 on error)
> -    pub used: i64,
> +    /// The Size of the underlying storage in bytes.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub total: Option<i64>,
> +    /// The used bytes of the underlying storage.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub used: Option<i64>,
>      /// The available bytes of the underlying storage. (-1 on error)
> -    pub avail: i64,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub avail: Option<i64>,
>      /// A list of usages of the past (last Month).
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub history: Option<Vec<Option<f64>>>,
> @@ -1335,9 +1338,9 @@ impl DataStoreStatusListItem {
>      pub fn empty(store: &str, err: Option<String>) -> Self {
>          DataStoreStatusListItem {
>              store: store.to_owned(),
> -            total: -1,
> -            used: -1,
> -            avail: -1,
> +            total: None,
> +            used: None,
> +            avail: None,
>              history: None,
>              history_start: None,
>              history_delta: None,
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index ff7d4cbd..6e758187 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -68,9 +68,9 @@ pub async fn datastore_status(
>  
>          let mut entry = DataStoreStatusListItem {
>              store: store.clone(),
> -            total: status.total as i64,
> -            used: status.used as i64,
> -            avail: status.available as i64,
> +            total: Some(status.total as i64),
> +            used: Some(status.used as i64),
> +            avail: Some(status.available as i64),
>              history: None,
>              history_start: None,
>              history_delta: None,
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Gabriel Goller]

On 12/7/23 18:33, Dietmar Maurer wrote:
> But why is it still i64? I would expect u64.
Yep, that makes more sense :)
I'll submit a v2.