From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0176162E2B for ; Wed, 23 Feb 2022 11:29:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E4D192CD53 for ; Wed, 23 Feb 2022 11:28:38 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 0F2EE2CD48 for ; Wed, 23 Feb 2022 11:28:37 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D321746C74 for ; Wed, 23 Feb 2022 11:28:36 +0100 (CET) Date: Wed, 23 Feb 2022 11:28:35 +0100 From: Wolfgang Bumiller To: Stefan Sterz Cc: pbs-devel@lists.proxmox.com Message-ID: <20220223102835.liwg3ylprgudeuhn@olga.proxmox.com> References: <20220222112556.3747239-1-s.sterz@proxmox.com> <20220222112556.3747239-4-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220222112556.3747239-4-s.sterz@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.377 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Feb 2022 10:29:09 -0000 some comments inline On Tue, Feb 22, 2022 at 12:25:54PM +0100, Stefan Sterz wrote: > add support for multiline comments to node.cfg, similar to how pve > handles multi-line comments > > Signed-off-by: Stefan Sterz > --- > pbs-api-types/src/lib.rs | 9 ++++++ > src/config/node.rs | 4 +-- > src/tools/config.rs | 62 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 73 insertions(+), 2 deletions(-) > > diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs > index 754e7b22..3892980d 100644 > --- a/pbs-api-types/src/lib.rs > +++ b/pbs-api-types/src/lib.rs > @@ -137,6 +137,8 @@ const_regex! { > > pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$"; > > + pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)\s*$"; I don't think the trailing `\s*` is necessary? > + > pub BACKUP_REPO_URL_REGEX = concat!( > r"^^(?:(?:(", > USER_ID_REGEX_STR!(), "|", APITOKEN_ID_REGEX_STR!(), > (...) > diff --git a/src/config/node.rs b/src/config/node.rs > index 9ca44a52..bb915f94 100644 > --- a/src/config/node.rs > +++ b/src/config/node.rs > @@ -9,7 +9,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater}; > use proxmox_http::ProxyConfig; > > use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA, > - SINGLE_LINE_COMMENT_SCHEMA}; > + MULTI_LINE_COMMENT_SCHEMA}; Please check how rustfmt would deal with the above `use` statement ;-) > use pbs_buildcfg::configdir; > use pbs_config::{open_backup_lockfile, BackupLockGuard}; > > (...) > diff --git a/src/tools/config.rs b/src/tools/config.rs > index f666a8ab..738ab541 100644 > --- a/src/tools/config.rs > +++ b/src/tools/config.rs > @@ -32,6 +32,20 @@ pub fn value_from_str(input: &str, schema: &'static Schema) -> Result > let mut config = Object::new(); > > + // parse first n lines starting with '#' as multi-line comment > + let comment = input.lines() > + .take_while(|l| l.starts_with('#')) > + .map(|l| { > + let mut ch = l.chars(); > + ch.next(); > + ch.as_str() ^ The `take_while` ensures `l` starts with '#', so you could just use .map(|l| &l[1..]) Alternatively, since 1.57 (and we're at 1.58 now), you could also combine the `.take_while` and `.map` into: .map_while(|l| l.strip_prefix("#")) However... > + }) > + .fold(String::new(), |acc, l| acc + l + "\n"); > + > + if !comment.is_empty() { > + config.insert("comment".to_string(), Value::String(comment)); > + } > + > for (lineno, line) in input.lines().enumerate() { ... here we're starting over, so maybe we should refactor this a little. Eg. we could use a `.lines().enumerate().peekable()` iterator: let mut lines = input.lines().enumerate().peekable(); let mut comments = String::new(); while let Some((_, line)) = iter.next_if(|(_, line)| line.starts_with('#') { comments.push_str(&line[1..]); comments.push('\n'); } for (lineno, line) in lines { <...> > let line = line.trim(); > if line.starts_with('#') || line.is_empty() { > @@ -133,10 +147,23 @@ pub fn value_to_bytes(value: &Value, schema: &'static Schema) -> Result, > > /// Note: the object must have already been verified at this point. > fn object_to_writer(output: &mut dyn Write, object: &Object) -> Result<(), Error> { > + // special key `comment` for multi-line notes > + if object.contains_key("comment") { > + let comment = match object.get("comment") { `contains_key` + `get` is somewhat wasteful and should be combined. if let Some(comment) = object.get("comment") { For the type check you can then use `.as_str().ok_or_else(...)` Or alternatively use a single match for both checks: match object.get("comment") { Some(Value::String(comment)) => { } Some(_) => bail!(...), None => (), } > + Some(Value::String(v)) => v, > + _ => bail!("only strings can be comments"), > + }; > + > + for lines in comment.lines() { > + writeln!(output, "#{}", lines)?; > + } > + } > + > for (key, value) in object.iter() { Given that we type-check the comment above _and_ the data matching the schema is a precondition for calling this function, I'd just put an if key == "comment" { continue } here rather than the conditional check limited to the `Value::String` case below. > match value { > Value::Null => continue, // delete this entry > Value::Bool(v) => writeln!(output, "{}: {}", key, v)?, > + Value::String(_) if key == "comment" => continue, // skip comment as we handle it above > Value::String(v) => { > if v.as_bytes().contains(&b'\n') { > bail!("value for {} contains newlines", key); > @@ -172,3 +199,38 @@ fn test() { > > assert_eq!(config, NODE_CONFIG.as_bytes()); > } > + > +#[test] > +fn test_with_comment() { > + use proxmox_schema::ApiType; > + > + // let's just reuse some schema we actually have available: > + use crate::config::node::NodeConfig; > + > + const NODE_INPUT: &str = "\ > + #this should\n\ > + #be included\n\ > + acme: account=pebble\n\ > + # this should not\n\ ^ I find it curous that your 'comment' section doesn't have leading spaces, while the "real comment" does ;-) Should we think about trimming off up to 1 space when parsing the lines and prefix them with `"# "` when printing them out? Though I suppose it doesn't matter much as since we drop "real" comments on a RMW-cycle these files don't really need to bother much with being all that eye-pleasing I guess...