[pbs-devel] [PATCH proxmox v2 0/3] fix #6161: time: Split parse_time_spec parser into two

[pbs-devel] [PATCH proxmox v2 0/3] fix #6161: time: Split parse_time_spec parser into two

[Maximiliano Sandoval]

In a support case we found a system with 40:00 as a schedule in a backup job. It
is possible to entire this calendar event on the web UI.

As per systemd-analize:

```
$ systemd-analyze calendar 40:00
Failed to parse calendar specification '40:00': Invalid argument
```

After this series the Proxmox VE web UI won't allow it anymore with an error:

```
Parameter verification failed. (400)

schedule: invalid format - invalid calendar event '40:00' - unable to parse calendar event at ':00' - Nom(Eof) 
```

Differences from v1:
 - Add cover letter
 - Reference bugzilla entry #6161
 - Add test case for 24:00 as per #6161

Maximiliano Sandoval (3):
  time: Add traits to DateTimeValue and TimeSpec
  fix #6161: time: Split parse_time_spec parser into two
  time: Add more calendat event tests

 proxmox-time/src/calendar_event.rs  | 109 ++++++++++++++++++++++++++--
 proxmox-time/src/date_time_value.rs |   2 +-
 2 files changed, 103 insertions(+), 8 deletions(-)

-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 1/3] time: Add traits to DateTimeValue and TimeSpec

[Maximiliano Sandoval]

which are useful for tests.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 proxmox-time/src/calendar_event.rs  | 1 +
 proxmox-time/src/date_time_value.rs | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/proxmox-time/src/calendar_event.rs b/proxmox-time/src/calendar_event.rs
index 696a11a2..5d5bdf83 100644
--- a/proxmox-time/src/calendar_event.rs
+++ b/proxmox-time/src/calendar_event.rs
@@ -341,6 +341,7 @@ fn parse_calendar_event_incomplete(mut i: &str) -> IResult<&str, CalendarEvent>
     Ok((i, event))
 }
 
+#[derive(Debug, PartialEq)]
 struct TimeSpec {
     hour: Vec<DateTimeValue>,
     minute: Vec<DateTimeValue>,
diff --git a/proxmox-time/src/date_time_value.rs b/proxmox-time/src/date_time_value.rs
index 4dd79d36..08692b38 100644
--- a/proxmox-time/src/date_time_value.rs
+++ b/proxmox-time/src/date_time_value.rs
@@ -1,4 +1,4 @@
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub(crate) enum DateTimeValue {
     Single(u32),
     Range(u32, u32),
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 2/3] fix #6161: time: Split parse_time_spec parser into two

[Maximiliano Sandoval]

When using a single parser "40:00" would be parsed as "40 minutes and 00
seconds", but this should error out:

```
$ systemd-analyze calendar 40:00
Failed to parse calendar specification '40:00': Invalid argument
```

We split into two parsers one in which seconds are specified and one in
which they are not and add tests for good measure.

The test added in this commit would not error out before this commit and
was erroneously producing "Every 40 minutes" as a result.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 proxmox-time/src/calendar_event.rs | 80 +++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/proxmox-time/src/calendar_event.rs b/proxmox-time/src/calendar_event.rs
index 5d5bdf83..6fe453be 100644
--- a/proxmox-time/src/calendar_event.rs
+++ b/proxmox-time/src/calendar_event.rs
@@ -1,5 +1,6 @@
 use anyhow::Error;
 use nom::{
+    branch::alt,
     bytes::complete::tag,
     character::complete::space0,
     combinator::opt,
@@ -397,16 +398,13 @@ fn parse_date_time_comp_list(
     }
 }
 
-fn parse_time_spec(i: &str) -> IResult<&str, TimeSpec> {
-    let (i, (opt_hour, minute, opt_second)) = tuple((
-        opt(terminated(parse_date_time_comp_list(0, 24), tag(":"))),
+fn parse_time_spec_with_seconds(i: &str) -> IResult<&str, TimeSpec> {
+    let (i, (hour, minute, second)) = tuple((
+        terminated(parse_date_time_comp_list(0, 24), tag(":")),
         parse_date_time_comp_list(0, 60),
-        opt(preceded(tag(":"), parse_date_time_comp_list(0, 60))),
+        preceded(tag(":"), parse_date_time_comp_list(0, 60)),
     ))(i)?;
 
-    let hour = opt_hour.unwrap_or_default();
-    let second = opt_second.unwrap_or_else(|| vec![DateTimeValue::Single(0)]);
-
     Ok((
         i,
         TimeSpec {
@@ -417,6 +415,32 @@ fn parse_time_spec(i: &str) -> IResult<&str, TimeSpec> {
     ))
 }
 
+fn parse_time_spec_without_seconds(i: &str) -> IResult<&str, TimeSpec> {
+    let (i, (opt_hour, minute)) = tuple((
+        opt(terminated(parse_date_time_comp_list(0, 24), tag(":"))),
+        parse_date_time_comp_list(0, 60),
+    ))(i)?;
+
+    let hour = opt_hour.unwrap_or_default();
+    let second = vec![DateTimeValue::Single(0)];
+
+    Ok((
+        i,
+        TimeSpec {
+            hour,
+            minute,
+            second,
+        },
+    ))
+}
+
+fn parse_time_spec(i: &str) -> IResult<&str, TimeSpec> {
+    alt((
+        parse_time_spec_with_seconds,
+        parse_time_spec_without_seconds,
+    ))(i)
+}
+
 fn parse_date_spec(i: &str) -> IResult<&str, DateSpec> {
     // TODO: implement ~ for days (man systemd.time)
     if let Ok((i, (year, month, day))) = tuple((
@@ -443,3 +467,45 @@ fn parse_date_spec(i: &str) -> IResult<&str, DateSpec> {
         Err(parse_error(i, "invalid date spec"))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_calendar_event_from_str() {
+        use std::str::FromStr;
+
+        assert!(CalendarEvent::from_str("15:30").is_ok());
+
+        assert!(CalendarEvent::from_str("24:00").is_err());
+        assert!(CalendarEvent::from_str("40:00").is_err());
+    }
+
+    #[test]
+    fn test_parse_time_spec() {
+        let (input, data) = parse_time_spec("15:30").unwrap();
+        assert_eq!(input, "");
+        assert_eq!(
+            data,
+            TimeSpec {
+                hour: vec![DateTimeValue::Single(15)],
+                minute: vec![DateTimeValue::Single(30)],
+                second: vec![DateTimeValue::Single(0)]
+            }
+        );
+
+        // These do not strictly fail, but should fail when parsed with FromStr
+        // since there will be input left to consume.
+        let (input, data) = parse_time_spec("40:00").unwrap();
+        assert_eq!(input, ":00");
+        assert_eq!(
+            data,
+            TimeSpec {
+                hour: vec![],
+                minute: vec![DateTimeValue::Single(40)],
+                second: vec![DateTimeValue::Single(0)]
+            }
+        );
+    }
+}
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 3/3] time: Add more calendat event tests

[Maximiliano Sandoval]

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 proxmox-time/src/calendar_event.rs | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/proxmox-time/src/calendar_event.rs b/proxmox-time/src/calendar_event.rs
index 6fe453be..6a158b7c 100644
--- a/proxmox-time/src/calendar_event.rs
+++ b/proxmox-time/src/calendar_event.rs
@@ -477,9 +477,15 @@ mod tests {
         use std::str::FromStr;
 
         assert!(CalendarEvent::from_str("15:30").is_ok());
+        assert!(CalendarEvent::from_str("0").is_ok());
+        assert!(CalendarEvent::from_str("59").is_ok());
+        assert!(CalendarEvent::from_str("15:30:59").is_ok());
 
         assert!(CalendarEvent::from_str("24:00").is_err());
         assert!(CalendarEvent::from_str("40:00").is_err());
+        assert!(CalendarEvent::from_str("60").is_err());
+        assert!(CalendarEvent::from_str("24:61").is_err());
+        assert!(CalendarEvent::from_str("15:30:60").is_err());
     }
 
     #[test]
@@ -495,6 +501,10 @@ mod tests {
             }
         );
 
+        assert!(parse_time_spec("1:00:00").is_ok());
+        assert!(parse_time_spec("1:00").is_ok());
+        assert!(parse_time_spec("59").is_ok());
+
         // These do not strictly fail, but should fail when parsed with FromStr
         // since there will be input left to consume.
         let (input, data) = parse_time_spec("40:00").unwrap();
@@ -507,5 +517,23 @@ mod tests {
                 second: vec![DateTimeValue::Single(0)]
             }
         );
+
+        assert!(parse_time_spec("61").is_err());
+        // This should be 1:00 instead.
+        assert!(parse_time_spec("60").is_err());
+
+        // Can only partially parse
+        let (input, _data) = parse_time_spec("24:61").unwrap();
+        assert_eq!(input, ":61");
+
+
+        assert_eq!(
+            data,
+            TimeSpec {
+                hour: vec![],
+                minute: vec![DateTimeValue::Single(40)],
+                second: vec![DateTimeValue::Single(0)]
+            }
+        );
     }
 }
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v2 0/3] fix #6161: time: Split parse_time_spec parser into two

[Thomas Lamprecht]

On 13/08/2025 14:43, Maximiliano Sandoval wrote:
> In a support case we found a system with 40:00 as a schedule in a backup job. It
> is possible to entire this calendar event on the web UI.
> 
> As per systemd-analize:
> 
> ```
> $ systemd-analyze calendar 40:00
> Failed to parse calendar specification '40:00': Invalid argument
> ```
> 
> After this series the Proxmox VE web UI won't allow it anymore with an error:
> 
> ```
> Parameter verification failed. (400)
> 
> schedule: invalid format - invalid calendar event '40:00' - unable to parse calendar event at ':00' - Nom(Eof) 
> ```
> 

What about the backward compat concerns from Dominik and Fiona, I see
nothing written anywhere in this series addressing them, or did I just
overlooked that?

Could we treat only serializing strict and deserializing not?

> Differences from v1:
>  - Add cover letter
>  - Reference bugzilla entry #6161
>  - Add test case for 24:00 as per #6161
> 
> Maximiliano Sandoval (3):
>   time: Add traits to DateTimeValue and TimeSpec
>   fix #6161: time: Split parse_time_spec parser into two
>   time: Add more calendat event tests
> 
>  proxmox-time/src/calendar_event.rs  | 109 ++++++++++++++++++++++++++--
>  proxmox-time/src/date_time_value.rs |   2 +-
>  2 files changed, 103 insertions(+), 8 deletions(-)
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v2 0/3] fix #6161: time: Split parse_time_spec parser into two

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> On 13/08/2025 14:43, Maximiliano Sandoval wrote:
>> In a support case we found a system with 40:00 as a schedule in a backup job. It
>> is possible to entire this calendar event on the web UI.
>> 
>> As per systemd-analize:
>> 
>> ```
>> $ systemd-analyze calendar 40:00
>> Failed to parse calendar specification '40:00': Invalid argument
>> ```
>> 
>> After this series the Proxmox VE web UI won't allow it anymore with an error:
>> 
>> ```
>> Parameter verification failed. (400)
>> 
>> schedule: invalid format - invalid calendar event '40:00' - unable to parse calendar event at ':00' - Nom(Eof) 
>> ```
>> 
>
> What about the backward compat concerns from Dominik and Fiona, I see
> nothing written anywhere in this series addressing them, or did I just
> overlooked that?
>
> Could we treat only serializing strict and deserializing not?

I am not sure if it is possible or something we do in general. We would
need to be strict when de-serializing from the web UI (to prevent wrong
entries from reaching the configuration file) but be more lenient when
de-serializing from the configuration file.

>> Differences from v1:
>>  - Add cover letter
>>  - Reference bugzilla entry #6161
>>  - Add test case for 24:00 as per #6161
>> 
>> Maximiliano Sandoval (3):
>>   time: Add traits to DateTimeValue and TimeSpec
>>   fix #6161: time: Split parse_time_spec parser into two
>>   time: Add more calendat event tests
>> 
>>  proxmox-time/src/calendar_event.rs  | 109 ++++++++++++++++++++++++++--
>>  proxmox-time/src/date_time_value.rs |   2 +-
>>  2 files changed, 103 insertions(+), 8 deletions(-)
>> 

-- 
Maximiliano


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v2 0/3] fix #6161: time: Split parse_time_spec parser into two

[Thomas Lamprecht]

On 27/08/2025 10:40, Maximiliano Sandoval wrote:
>> What about the backward compat concerns from Dominik and Fiona, I see
>> nothing written anywhere in this series addressing them, or did I just
>> overlooked that?
>>
>> Could we treat only serializing strict and deserializing not?
> I am not sure if it is possible or something we do in general. We would
> need to be strict when de-serializing from the web UI (to prevent wrong
> entries from reaching the configuration file) but be more lenient when
> de-serializing from the configuration file.
> 

Yes, that's the idea ;-) It is a pattern we employed quite a few times
in the past already.
I.e., accept and directly map legacy variants to the "good format" on
parse, then when it gets serialized out again it will be automatically
in the modern "correct" format.

As all that was parsed got transformed to new one can safely block any
legacy variant on serialization, as there such old variants must come
from the API then. Rejecting this in the UI already is rather extra
convenience that is nice to have but not a bare necessity.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel