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

[pbs-devel] [PATCH proxmox 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 2/3] 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 | 79 +++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 7 deletions(-)

diff --git a/proxmox-time/src/calendar_event.rs b/proxmox-time/src/calendar_event.rs
index 5d5bdf83..edb258ba 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,44 @@ 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("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 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 edb258ba..8222bcec 100644
--- a/proxmox-time/src/calendar_event.rs
+++ b/proxmox-time/src/calendar_event.rs
@@ -477,8 +477,14 @@ 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("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]
@@ -494,6 +500,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();
@@ -506,5 +516,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 1/3] time: Add traits to DateTimeValue and TimeSpec

[Maximiliano Sandoval]

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> 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(-)

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.

After this series the 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) 
```

-- 
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 1/3] time: Add traits to DateTimeValue and TimeSpec

[Maximiliano Sandoval]

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> 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(-)

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.

After this series the 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) 
```

-- 
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 1/3] time: Add traits to DateTimeValue and TimeSpec

[Dominik Csapak]

On 8/13/25 13:26, Maximiliano Sandoval wrote:
> 
> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
> 
>> 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(-)
> 
> 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.
> 
> After this series the 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)
> ```
> 

but that is a valid calendar spec? it just means something different 
than what the user expected ? (every hour at minute 40)


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

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

[Maximiliano Sandoval]

Dominik Csapak <d.csapak@proxmox.com> writes:

> On 8/13/25 13:26, Maximiliano Sandoval wrote:
>> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
>> 
>>> 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(-)
>> 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.
>> After this series the 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)
>> ```
>> 
>
> but that is a valid calendar spec? it just means something different than what
> the user expected ? (every hour at minute 40)

At least systemd-analyze does not like it:

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

I would hope that 20:00 is no different than 40:00 (e.g. the first
component refers to hours). Is the "calendar spec" defined somewhere?

-- 
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 1/3] time: Add traits to DateTimeValue and TimeSpec

[Dominik Csapak]

On 8/13/25 13:29, Maximiliano Sandoval wrote:
> Dominik Csapak <d.csapak@proxmox.com> writes:
> 
>> On 8/13/25 13:26, Maximiliano Sandoval wrote:
>>> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
>>>
>>>> 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(-)
>>> 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.
>>> After this series the 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)
>>> ```
>>>
>>
>> but that is a valid calendar spec? it just means something different than what
>> the user expected ? (every hour at minute 40)
> 
> At least systemd-analyze does not like it:
> 
> ```
> systemd-analyze calendar 40:00
> Failed to parse calendar specification '40:00': Invalid argument
> ```
> 

ok fair point


> I would hope that 20:00 is no different than 40:00 (e.g. the first
> component refers to hours). Is the "calendar spec" defined somewhere?
> 


we have this:
https://pbs.proxmox.com/docs/calendarevents.html

which actually supports your change (hours not optional in the timespec)

i just fear that some people already have configures such jobs that will 
not run anymore after this change?


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

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

[Fiona Ebner]

Am 13.08.25 um 1:26 PM schrieb Maximiliano Sandoval:
> 
> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
> 
>> 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(-)
> 
> 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.
> 
> After this series the 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) 
> ```
> 

Note that we have the same behavior in Proxmox VE:
https://bugzilla.proxmox.com/show_bug.cgi?id=6161

If it is changed, it should be made consistent between products.
Unfortunately, this one got missed before the major releases, as one
could consider this a breaking change.


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

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

[Maximiliano Sandoval]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 13.08.25 um 1:26 PM schrieb Maximiliano Sandoval:
>> 
>> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
>> 
>>> 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(-)
>> 
>> 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.
>> 
>> After this series the 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) 
>> ```
>> 
>
> Note that we have the same behavior in Proxmox VE:
> https://bugzilla.proxmox.com/show_bug.cgi?id=6161
>
> If it is changed, it should be made consistent between products.
> Unfortunately, this one got missed before the major releases, as one
> could consider this a breaking change.

The Proxmox VE web UI uses this crate so it would be consistent, I
tested the change against it.

I can do a v2 referencing the bugzilla entry, and add a test for "24:00"
which returns an error with this patch (as it should).

-- 
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 1/3] time: Add traits to DateTimeValue and TimeSpec

[Fiona Ebner]

Am 13.08.25 um 2:37 PM schrieb Maximiliano Sandoval:
> Fiona Ebner <f.ebner@proxmox.com> writes:
> 
>> Am 13.08.25 um 1:26 PM schrieb Maximiliano Sandoval:
>>>
>>> Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
>>>
>>>> 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(-)
>>>
>>> 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.
>>>
>>> After this series the 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) 
>>> ```
>>>
>>
>> Note that we have the same behavior in Proxmox VE:
>> https://bugzilla.proxmox.com/show_bug.cgi?id=6161
>>
>> If it is changed, it should be made consistent between products.
>> Unfortunately, this one got missed before the major releases, as one
>> could consider this a breaking change.
> 
> The Proxmox VE web UI uses this crate so it would be consistent, I
> tested the change against it.
> 
> I can do a v2 referencing the bugzilla entry, and add a test for "24:00"
> which returns an error with this patch (as it should).

Ok, great! But I'm not sure if this change should be done right now.
Again, this can break existing jobs!


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

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

[Maximiliano Sandoval]

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> which are useful for tests.
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

Superseded by:
https://lore.proxmox.com/all/20250813124347.452585-1-m.sandoval@proxmox.com/

-- 
Maximiliano


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