From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 5F2F31FF13F for ; Thu, 07 May 2026 11:16:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 27D1A10D71; Thu, 7 May 2026 11:16:52 +0200 (CEST) Message-ID: Date: Thu, 7 May 2026 11:16:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 1/2] zfs: status: add `VDevStats` struct instead of 3 optional u64s To: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pbs-devel@lists.proxmox.com References: <20260505141606.438908-1-n.frey@proxmox.com> <20260505141606.438908-2-n.frey@proxmox.com> <1778140433.30e1wc8usw.astroid@yuna.none> Content-Language: en-US From: Nicolas Frey In-Reply-To: <1778140433.30e1wc8usw.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: 1778145269055 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.767 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 Message-ID-Hash: 3ZDBB226NU5MM3SBQJNXIGTT47SX75OD X-Message-ID-Hash: 3ZDBB226NU5MM3SBQJNXIGTT47SX75OD 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 CC: Thomas Lamprecht 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: >> 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 >> Signed-off-by: Nicolas Frey >> --- >> 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, >> #[serde(skip_serializing_if = "Option::is_none")] >> - pub read: Option, >> - #[serde(skip_serializing_if = "Option::is_none")] >> - pub write: Option, >> - #[serde(skip_serializing_if = "Option::is_none")] >> - pub cksum: Option, >> + #[serde(flatten)] >> + pub stats: Option, >> #[serde(skip_serializing_if = "Option::is_none")] >> pub msg: Option, >> } > > 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 >> >> >> >>