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

[Nicolas Frey <n.frey@proxmox.com>]

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
>>
>>
>>
>>