From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id DB7481FF13F for ; Thu, 07 May 2026 11:03:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C358310A47; Thu, 7 May 2026 11:03:26 +0200 (CEST) Message-ID: <895d2f5c-881c-44f3-859f-7bf9a3b0fc7a@proxmox.com> Date: Thu, 7 May 2026 11:03:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 2/2] fix #7541: zfs status: account for msg directly after status in vdev parser To: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pbs-devel@lists.proxmox.com References: <20260505141606.438908-1-n.frey@proxmox.com> <20260505141606.438908-3-n.frey@proxmox.com> <1778140573.9y7e3sqjfm.astroid@yuna.none> Content-Language: en-US From: Nicolas Frey In-Reply-To: <1778140573.9y7e3sqjfm.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778144492245 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.770 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Message-ID-Hash: X7KX6XTGRXSZ6NUOFSOKLL4NAAAHVRVX X-Message-ID-Hash: X7KX6XTGRXSZ6NUOFSOKLL4NAAAHVRVX X-MailFrom: n.frey@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 >> --- >> 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 >> >> >> >>