Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <s.sterz@proxmox.com>
> ---
>  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<Value, Err
>  
>      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<Vec<u8>,
>  
>  /// 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)) => {
            <the loop>
        }
        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...