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

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