[PATCH proxmox-backup v2 0/2] fix #7541: zfs status: account for msg directly after status in vdev parser

[PATCH proxmox-backup v2 0/2] fix #7541: zfs status: account for msg directly after status in vdev parser

[Nicolas Frey]

As reported in Enterprise Support, when a spare is "INUSE", it has a
`msg` right after the STATUS, which the parser previously did not
account for.

Changes since v1:
* introduce new struct `VDevStats` to reduce boilerplate

proxmox-backup:

Nicolas Frey (2):
  zfs: status: add `VDevStats` struct instead of 3 optional u64s
  fix #7541: zfs status: account for msg directly after status in vdev
    parser

 src/tools/disks/zpool_status.rs | 51 +++++++++++++++++----------------
 1 file changed, 26 insertions(+), 25 deletions(-)


Summary over all repositories:
  1 files changed, 26 insertions(+), 25 deletions(-)

--
Generated by git-murpp 0.8.1

[PATCH proxmox-backup v2 1/2] zfs: status: add `VDevStats` struct instead of 3 optional u64s

[Nicolas Frey]

in order to reduce a bit of boilerplate. Keeps (de)serialization the
same by flattening `stats`. No functional change intended

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
New in v2
 src/tools/disks/zpool_status.rs | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
index 05ef5d80..9427f6d4 100644
--- a/src/tools/disks/zpool_status.rs
+++ b/src/tools/disks/zpool_status.rs
@@ -24,15 +24,19 @@ pub struct ZFSPoolVDevState {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub state: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub read: Option<u64>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub write: Option<u64>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub cksum: Option<u64>,
+    #[serde(flatten)]
+    pub stats: Option<VDevStats>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub msg: Option<String>,
 }

+#[derive(Debug, Serialize, Deserialize)]
+pub struct VDevStats {
+    read: u64,
+    write: u64,
+    cksum: u64,
+}
+
 fn expand_tab_length(input: &str) -> usize {
     input.chars().map(|c| if c == '\t' { 8 } else { 1 }).sum()
 }
@@ -57,9 +61,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
             name: vdev_name.to_string(),
             lvl: indent_level,
             state: None,
-            read: None,
-            write: None,
-            cksum: None,
+            stats: None,
             msg: None,
         };
         return Ok((n, vdev));
@@ -72,9 +74,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
             name: vdev_name.to_string(),
             lvl: indent_level,
             state: Some(state.to_string()),
-            read: None,
-            write: None,
-            cksum: None,
+            stats: None,
             msg: None,
         };
         return Ok((n, vdev));
@@ -90,9 +90,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
         name: vdev_name.to_string(),
         lvl: indent_level,
         state: Some(state.to_string()),
-        read: Some(read),
-        write: Some(write),
-        cksum: Some(cksum),
+        stats: Some(VDevStats { read, write, cksum }),
         msg: msg.map(String::from),
     };

@@ -283,9 +281,7 @@ fn test_vdev_list_to_tree() {
         name: String::new(),
         lvl: 0,
         state: None,
-        read: None,
-        write: None,
-        cksum: None,
+        stats: None,
         msg: None,
     };

--
2.47.3

Re: [PATCH proxmox-backup v2 1/2] zfs: status: add `VDevStats` struct instead of 3 optional u64s

[Fabian Grünbichler]

On May 5, 2026 4:16 pm, Nicolas Frey wrote:
> in order to reduce a bit of boilerplate. Keeps (de)serialization the
> same by flattening `stats`. No functional change intended
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
> New in v2
>  src/tools/disks/zpool_status.rs | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
> index 05ef5d80..9427f6d4 100644
> --- a/src/tools/disks/zpool_status.rs
> +++ b/src/tools/disks/zpool_status.rs
> @@ -24,15 +24,19 @@ pub struct ZFSPoolVDevState {
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub state: Option<String>,
>      #[serde(skip_serializing_if = "Option::is_none")]
> -    pub read: Option<u64>,
> -    #[serde(skip_serializing_if = "Option::is_none")]
> -    pub write: Option<u64>,
> -    #[serde(skip_serializing_if = "Option::is_none")]
> -    pub cksum: Option<u64>,
> +    #[serde(flatten)]
> +    pub stats: Option<VDevStats>,
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub msg: Option<String>,
>  }

this is also duplicated/already moved to proxmox-disks, and changing it
there would be a breaking change since the struct is not only consumed
via deserialization, but crosses package/crate boundaries..

so if we do it, we should also do it there - if we want to avoid that
churn, we need to keep the actual members separate.

> 
> +#[derive(Debug, Serialize, Deserialize)]
> +pub struct VDevStats {
> +    read: u64,
> +    write: u64,
> +    cksum: u64,
> +}
> +
>  fn expand_tab_length(input: &str) -> usize {
>      input.chars().map(|c| if c == '\t' { 8 } else { 1 }).sum()
>  }
> @@ -57,9 +61,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>              name: vdev_name.to_string(),
>              lvl: indent_level,
>              state: None,
> -            read: None,
> -            write: None,
> -            cksum: None,
> +            stats: None,
>              msg: None,
>          };
>          return Ok((n, vdev));
> @@ -72,9 +74,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>              name: vdev_name.to_string(),
>              lvl: indent_level,
>              state: Some(state.to_string()),
> -            read: None,
> -            write: None,
> -            cksum: None,
> +            stats: None,
>              msg: None,
>          };
>          return Ok((n, vdev));
> @@ -90,9 +90,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>          name: vdev_name.to_string(),
>          lvl: indent_level,
>          state: Some(state.to_string()),
> -        read: Some(read),
> -        write: Some(write),
> -        cksum: Some(cksum),
> +        stats: Some(VDevStats { read, write, cksum }),
>          msg: msg.map(String::from),
>      };
> 
> @@ -283,9 +281,7 @@ fn test_vdev_list_to_tree() {
>          name: String::new(),
>          lvl: 0,
>          state: None,
> -        read: None,
> -        write: None,
> -        cksum: None,
> +        stats: None,
>          msg: None,
>      };
> 
> --
> 2.47.3
> 
> 
> 
> 

Re: [PATCH proxmox-backup v2 1/2] zfs: status: add `VDevStats` struct instead of 3 optional u64s

[Nicolas Frey]

On 5/7/26 9:58 AM, Fabian Grünbichler wrote:
> On May 5, 2026 4:16 pm, Nicolas Frey wrote:
>> in order to reduce a bit of boilerplate. Keeps (de)serialization the
>> same by flattening `stats`. No functional change intended
>>
>> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>> New in v2
>>  src/tools/disks/zpool_status.rs | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
>> index 05ef5d80..9427f6d4 100644
>> --- a/src/tools/disks/zpool_status.rs
>> +++ b/src/tools/disks/zpool_status.rs
>> @@ -24,15 +24,19 @@ pub struct ZFSPoolVDevState {
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub state: Option<String>,
>>      #[serde(skip_serializing_if = "Option::is_none")]
>> -    pub read: Option<u64>,
>> -    #[serde(skip_serializing_if = "Option::is_none")]
>> -    pub write: Option<u64>,
>> -    #[serde(skip_serializing_if = "Option::is_none")]
>> -    pub cksum: Option<u64>,
>> +    #[serde(flatten)]
>> +    pub stats: Option<VDevStats>,
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub msg: Option<String>,
>>  }
> 
> this is also duplicated/already moved to proxmox-disks, and changing it
> there would be a breaking change since the struct is not only consumed
> via deserialization, but crosses package/crate boundaries..
> 
> so if we do it, we should also do it there - if we want to avoid that
> churn, we need to keep the actual members separate.
> 

Ah thanks! I was unaware that this code was duplicated to
proxmox-disks. I reckon the place to fix the parsing would rather be
there then? Or should it be fixed on both sides, until PBS uses
proxmox-disks?

I don't believe we gain much from pulling this out into a separate
struct here after thinking about it (especially with what you
mentioned here), I'll drop it in the next version

>>
>> +#[derive(Debug, Serialize, Deserialize)]
>> +pub struct VDevStats {
>> +    read: u64,
>> +    write: u64,
>> +    cksum: u64,
>> +}
>> +
>>  fn expand_tab_length(input: &str) -> usize {
>>      input.chars().map(|c| if c == '\t' { 8 } else { 1 }).sum()
>>  }
>> @@ -57,9 +61,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>>              name: vdev_name.to_string(),
>>              lvl: indent_level,
>>              state: None,
>> -            read: None,
>> -            write: None,
>> -            cksum: None,
>> +            stats: None,
>>              msg: None,
>>          };
>>          return Ok((n, vdev));
>> @@ -72,9 +74,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>>              name: vdev_name.to_string(),
>>              lvl: indent_level,
>>              state: Some(state.to_string()),
>> -            read: None,
>> -            write: None,
>> -            cksum: None,
>> +            stats: None,
>>              msg: None,
>>          };
>>          return Ok((n, vdev));
>> @@ -90,9 +90,7 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>>          name: vdev_name.to_string(),
>>          lvl: indent_level,
>>          state: Some(state.to_string()),
>> -        read: Some(read),
>> -        write: Some(write),
>> -        cksum: Some(cksum),
>> +        stats: Some(VDevStats { read, write, cksum }),
>>          msg: msg.map(String::from),
>>      };
>>
>> @@ -283,9 +281,7 @@ fn test_vdev_list_to_tree() {
>>          name: String::new(),
>>          lvl: 0,
>>          state: None,
>> -        read: None,
>> -        write: None,
>> -        cksum: None,
>> +        stats: None,
>>          msg: None,
>>      };
>>
>> --
>> 2.47.3
>>
>>
>>
>>

Re: [PATCH proxmox-backup v2 1/2] zfs: status: add `VDevStats` struct instead of 3 optional u64s

[Thomas Lamprecht]

Am 07.05.26 um 11:14 schrieb Nicolas Frey:
>> so if we do it, we should also do it there - if we want to avoid that
>> churn, we need to keep the actual members separate.
>>
> Ah thanks! I was unaware that this code was duplicated to
> proxmox-disks. I reckon the place to fix the parsing would rather be
> there then? Or should it be fixed on both sides, until PBS uses
> proxmox-disks?
> 
> I don't believe we gain much from pulling this out into a separate
> struct here after thinking about it (especially with what you
> mentioned here), I'll drop it in the next version

I owe moving it out of PBS and replacing it with the refactored out
proxmox-disks one. FWIW, the latter currently has not really that many
(any?) users (i.e., in a bumped package), so now we could still break this
without _that_ much pain, FWIW.

[PATCH proxmox-backup v2 2/2] fix #7541: zfs status: account for msg directly after status in vdev parser

[Nicolas Frey]

so spares that have the status `INUSE` parse correctly.

adds a line to the mock config in the `test_zpool_status_parser_spares`
test containing an example of a vdev that would be `INUSE`.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7541
Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
No functional changes since v1
 src/tools/disks/zpool_status.rs | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
index 9427f6d4..267fb916 100644
--- a/src/tools/disks/zpool_status.rs
+++ b/src/tools/disks/zpool_status.rs
@@ -14,7 +14,7 @@ use nom::{
     character::complete::line_ending,
     combinator::opt,
     multi::{many0, many1},
-    sequence::preceded,
+    sequence::{preceded, tuple},
 };

 #[derive(Debug, Serialize, Deserialize)]
@@ -68,29 +68,33 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
     }

     let (i, state) = preceded(multispace1, notspace1)(i)?;
-    if let Ok((n, _)) = preceded(multispace0, line_ending)(i) {
-        // spares
+    if let Ok((n, (read, write, cksum, msg, _))) = tuple((
+        preceded(multispace1, parse_u64),
+        preceded(multispace1, parse_u64),
+        preceded(multispace1, parse_u64),
+        opt(preceded(multispace1, take_while1(|c: char| c != '\n'))),
+        line_ending,
+    ))(i)
+    {
         let vdev = ZFSPoolVDevState {
             name: vdev_name.to_string(),
             lvl: indent_level,
             state: Some(state.to_string()),
-            stats: None,
-            msg: None,
+            stats: Some(VDevStats { read, write, cksum }),
+            msg: msg.map(String::from),
         };
         return Ok((n, vdev));
     }

-    let (i, read) = preceded(multispace1, parse_u64)(i)?;
-    let (i, write) = preceded(multispace1, parse_u64)(i)?;
-    let (i, cksum) = preceded(multispace1, parse_u64)(i)?;
-    let (i, msg) = opt(preceded(multispace1, take_while(|c| c != '\n')))(i)?;
+    // spares
+    let (i, msg) = opt(preceded(multispace1, take_while1(|c: char| c != '\n')))(i)?;
     let (i, _) = line_ending(i)?;

     let vdev = ZFSPoolVDevState {
         name: vdev_name.to_string(),
         lvl: indent_level,
         state: Some(state.to_string()),
-        stats: Some(VDevStats { read, write, cksum }),
+        stats: None,
         msg: msg.map(String::from),
     };

@@ -486,6 +490,7 @@ config:
         spares
           /dev/sdb     AVAIL
           /dev/sdc     AVAIL
+          /dev/sdd     INUSE    currently in use

 errors: No known data errors
 "###;
--
2.47.3

Re: [PATCH proxmox-backup v2 2/2] fix #7541: zfs status: account for msg directly after status in vdev parser

[Fabian Grünbichler]

On May 5, 2026 4:16 pm, Nicolas Frey wrote:
> so spares that have the status `INUSE` parse correctly.
> 
> adds a line to the mock config in the `test_zpool_status_parser_spares`
> test containing an example of a vdev that would be `INUSE`.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7541
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
> No functional changes since v1
>  src/tools/disks/zpool_status.rs | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
> index 9427f6d4..267fb916 100644
> --- a/src/tools/disks/zpool_status.rs
> +++ b/src/tools/disks/zpool_status.rs
> @@ -14,7 +14,7 @@ use nom::{
>      character::complete::line_ending,
>      combinator::opt,
>      multi::{many0, many1},
> -    sequence::preceded,
> +    sequence::{preceded, tuple},
>  };
> 
>  #[derive(Debug, Serialize, Deserialize)]
> @@ -68,29 +68,33 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>      }
> 
>      let (i, state) = preceded(multispace1, notspace1)(i)?;
> -    if let Ok((n, _)) = preceded(multispace0, line_ending)(i) {
> -        // spares
> +    if let Ok((n, (read, write, cksum, msg, _))) = tuple((
> +        preceded(multispace1, parse_u64),
> +        preceded(multispace1, parse_u64),
> +        preceded(multispace1, parse_u64),
> +        opt(preceded(multispace1, take_while1(|c: char| c != '\n'))),
> +        line_ending,
> +    ))(i)
> +    {
>          let vdev = ZFSPoolVDevState {
>              name: vdev_name.to_string(),
>              lvl: indent_level,
>              state: Some(state.to_string()),
> -            stats: None,
> -            msg: None,
> +            stats: Some(VDevStats { read, write, cksum }),
> +            msg: msg.map(String::from),

you misunderstood what I wrote as reply to v1.

we can actually parse the stats optionally first.

and then as next step parse the message.

and then as final step parse the line ending

and then have a single return statement :)

basically

// spares don't have stats
let stats = opt_parse_stats();

let msg = opt_parse_msg();
line_ending();
return

instead of

if (stats and msg and line_ending) {
   return
}

msg = ..
line_ending
return

the goal is to avoid the duplication of parsing the message and line
ending, and making the flow more natural, as that is how lines are
structured:

vdev state [stat stat stat] [msg]\n


>          };
>          return Ok((n, vdev));
>      }
> 
> -    let (i, read) = preceded(multispace1, parse_u64)(i)?;
> -    let (i, write) = preceded(multispace1, parse_u64)(i)?;
> -    let (i, cksum) = preceded(multispace1, parse_u64)(i)?;
> -    let (i, msg) = opt(preceded(multispace1, take_while(|c| c != '\n')))(i)?;
> +    // spares
> +    let (i, msg) = opt(preceded(multispace1, take_while1(|c: char| c != '\n')))(i)?;
>      let (i, _) = line_ending(i)?;
> 
>      let vdev = ZFSPoolVDevState {
>          name: vdev_name.to_string(),
>          lvl: indent_level,
>          state: Some(state.to_string()),
> -        stats: Some(VDevStats { read, write, cksum }),
> +        stats: None,
>          msg: msg.map(String::from),
>      };
> 
> @@ -486,6 +490,7 @@ config:
>          spares
>            /dev/sdb     AVAIL
>            /dev/sdc     AVAIL
> +          /dev/sdd     INUSE    currently in use
> 
>  errors: No known data errors
>  "###;
> --
> 2.47.3
> 
> 
> 
> 

Re: [PATCH proxmox-backup v2 2/2] fix #7541: zfs status: account for msg directly after status in vdev parser

[Nicolas Frey]

On 5/7/26 9:58 AM, Fabian Grünbichler wrote:
> On May 5, 2026 4:16 pm, Nicolas Frey wrote:
>> so spares that have the status `INUSE` parse correctly.
>>
>> adds a line to the mock config in the `test_zpool_status_parser_spares`
>> test containing an example of a vdev that would be `INUSE`.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7541
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>> No functional changes since v1
>>  src/tools/disks/zpool_status.rs | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/tools/disks/zpool_status.rs b/src/tools/disks/zpool_status.rs
>> index 9427f6d4..267fb916 100644
>> --- a/src/tools/disks/zpool_status.rs
>> +++ b/src/tools/disks/zpool_status.rs
>> @@ -14,7 +14,7 @@ use nom::{
>>      character::complete::line_ending,
>>      combinator::opt,
>>      multi::{many0, many1},
>> -    sequence::preceded,
>> +    sequence::{preceded, tuple},
>>  };
>>
>>  #[derive(Debug, Serialize, Deserialize)]
>> @@ -68,29 +68,33 @@ fn parse_zpool_status_vdev(i: &str) -> IResult<&str, ZFSPoolVDevState> {
>>      }
>>
>>      let (i, state) = preceded(multispace1, notspace1)(i)?;
>> -    if let Ok((n, _)) = preceded(multispace0, line_ending)(i) {
>> -        // spares
>> +    if let Ok((n, (read, write, cksum, msg, _))) = tuple((
>> +        preceded(multispace1, parse_u64),
>> +        preceded(multispace1, parse_u64),
>> +        preceded(multispace1, parse_u64),
>> +        opt(preceded(multispace1, take_while1(|c: char| c != '\n'))),
>> +        line_ending,
>> +    ))(i)
>> +    {
>>          let vdev = ZFSPoolVDevState {
>>              name: vdev_name.to_string(),
>>              lvl: indent_level,
>>              state: Some(state.to_string()),
>> -            stats: None,
>> -            msg: None,
>> +            stats: Some(VDevStats { read, write, cksum }),
>> +            msg: msg.map(String::from),
> 
> you misunderstood what I wrote as reply to v1.
> 
> we can actually parse the stats optionally first.
> 
> and then as next step parse the message.
> 
> and then as final step parse the line ending
> 
> and then have a single return statement :)
> 
> basically
> 
> // spares don't have stats
> let stats = opt_parse_stats();
> 
> let msg = opt_parse_msg();
> line_ending();
> return
> 
> instead of
> 
> if (stats and msg and line_ending) {
>    return
> }
> 
> msg = ..
> line_ending
> return
> 
> the goal is to avoid the duplication of parsing the message and line
> ending, and making the flow more natural, as that is how lines are
> structured:
> 
> vdev state [stat stat stat] [msg]\n
> 
> 

Argh im sorry I misunderstood, that actually makes a lot more sense.
I'll use this approach to make the code more readable in the next version

>>          };
>>          return Ok((n, vdev));
>>      }
>>
>> -    let (i, read) = preceded(multispace1, parse_u64)(i)?;
>> -    let (i, write) = preceded(multispace1, parse_u64)(i)?;
>> -    let (i, cksum) = preceded(multispace1, parse_u64)(i)?;
>> -    let (i, msg) = opt(preceded(multispace1, take_while(|c| c != '\n')))(i)?;
>> +    // spares
>> +    let (i, msg) = opt(preceded(multispace1, take_while1(|c: char| c != '\n')))(i)?;
>>      let (i, _) = line_ending(i)?;
>>
>>      let vdev = ZFSPoolVDevState {
>>          name: vdev_name.to_string(),
>>          lvl: indent_level,
>>          state: Some(state.to_string()),
>> -        stats: Some(VDevStats { read, write, cksum }),
>> +        stats: None,
>>          msg: msg.map(String::from),
>>      };
>>
>> @@ -486,6 +490,7 @@ config:
>>          spares
>>            /dev/sdb     AVAIL
>>            /dev/sdc     AVAIL
>> +          /dev/sdd     INUSE    currently in use
>>
>>  errors: No known data errors
>>  "###;
>> --
>> 2.47.3
>>
>>
>>
>>