[pbs-devel] [PATCH proxmox 0/5] time conversion tests and fixes

[pbs-devel] [PATCH proxmox 0/5] time conversion tests and fixes

[Fabian Grünbichler]

patches 1/2 could also be changed to not allow leap seconds when parsing
at all - which would no longer be RFC3339 conformant, but we are not
w.r.t some other things as well (e.g., t/z instead of T/Z).

Fabian Grünbichler (5):
  time: add test for leap second parsing/converting
  time: allow leap seconds when parsing RFC3339
  time: add tests for RFC3339 corner cases
  time/rfc3339: add leading zeroes for years < 1000
  time: add tests for gmtime range

 proxmox/src/tools/time.rs | 106 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 3 deletions(-)

-- 
2.20.1

[pbs-devel] [RFC proxmox 1/5] time: add test for leap second parsing/converting

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    test fails, fixed by next patch

 proxmox/src/tools/time.rs | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/proxmox/src/tools/time.rs b/proxmox/src/tools/time.rs
index 803c6c0..78d75b9 100644
--- a/proxmox/src/tools/time.rs
+++ b/proxmox/src/tools/time.rs
@@ -279,3 +279,26 @@ pub fn parse_rfc3339(i: &str) -> Result<i64, Error> {
         Ok(epoch)
     }).map_err(|err| format_err!("parse_rfc_3339 failed - {}", err))
 }
+
+#[test]
+fn test_leap_seconds() {
+    let convert_reconvert = |epoch| {
+        let rfc3339 = epoch_to_rfc3339_utc(epoch)
+            .expect("leap second epoch to rfc3339 should work");
+
+        let parsed = parse_rfc3339(&rfc3339)
+            .expect("parsing converted leap second epoch should work");
+
+        assert_eq!(epoch, parsed);
+    };
+
+    // 2005-12-31T23:59:59Z was followed by a leap second
+    let epoch = 1136073599;
+    convert_reconvert(epoch);
+    convert_reconvert(epoch + 1);
+    convert_reconvert(epoch + 2);
+
+    let parsed = parse_rfc3339("2005-12-31T23:59:60Z")
+        .expect("parsing leap second should work");
+    assert_eq!(parsed, epoch + 1);
+}
-- 
2.20.1

[pbs-devel] [RFC proxmox 2/5] time: allow leap seconds when parsing RFC3339

[Fabian Grünbichler]

we don't ever produce those, but they are valid RFC3339 strings

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    alternatively: print a nice error message for seconds == 60, these should only
    ever happen when we parse some user provided RFC3339 string..

 proxmox/src/tools/time.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox/src/tools/time.rs b/proxmox/src/tools/time.rs
index 78d75b9..f7faade 100644
--- a/proxmox/src/tools/time.rs
+++ b/proxmox/src/tools/time.rs
@@ -257,7 +257,7 @@ pub fn parse_rfc3339(i: &str) -> Result<i64, Error> {
         expect(13, b':')?;
         tm.set_min(check_max(digit(14)?*10 + digit(15)?, 59)?)?;
         expect(16, b':')?;
-        tm.set_sec(check_max(digit(17)?*10 + digit(18)?, 59)?)?;
+        tm.set_sec(check_max(digit(17)?*10 + digit(18)?, 60)?)?;
 
         let epoch = tm.into_epoch()?;
         if tz == b'Z' {
-- 
2.20.1

[pbs-devel] [PATCH proxmox 3/5] time: add tests for RFC3339 corner cases

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    test fails untils next patch

 proxmox/src/tools/time.rs | 54 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/proxmox/src/tools/time.rs b/proxmox/src/tools/time.rs
index f7faade..5747854 100644
--- a/proxmox/src/tools/time.rs
+++ b/proxmox/src/tools/time.rs
@@ -302,3 +302,57 @@ fn test_leap_seconds() {
         .expect("parsing leap second should work");
     assert_eq!(parsed, epoch + 1);
 }
+
+#[test]
+fn test_rfc3339_range() {
+    // also tests single-digit years/first decade values
+    let lower = -62167219200;
+    let lower_str = "0000-01-01T00:00:00Z";
+
+    let upper = 253402300799;
+    let upper_str = "9999-12-31T23:59:59Z";
+
+    let converted = epoch_to_rfc3339_utc(lower)
+        .expect("converting lower bound of RFC3339 range should work");
+    assert_eq!(converted, lower_str);
+
+    let converted = epoch_to_rfc3339_utc(upper)
+        .expect("converting upper bound of RFC3339 range should work");
+    assert_eq!(converted, upper_str);
+
+    let parsed = parse_rfc3339(lower_str)
+        .expect("parsing lower bound of RFC3339 range should work");
+    assert_eq!(parsed, lower);
+
+    let parsed = parse_rfc3339(upper_str)
+        .expect("parsing upper bound of RFC3339 range should work");
+    assert_eq!(parsed, upper);
+
+    epoch_to_rfc3339_utc(lower-1)
+        .expect_err("converting below lower bound of RFC3339 range should fail");
+
+    epoch_to_rfc3339_utc(upper+1)
+        .expect_err("converting above upper bound of RFC3339 range should fail");
+
+    let first_century = -59011459201;
+    let first_century_str = "0099-12-31T23:59:59Z";
+
+    let converted = epoch_to_rfc3339_utc(first_century)
+        .expect("converting epoch representing first century year should work");
+    assert_eq!(converted, first_century_str);
+
+    let parsed = parse_rfc3339(first_century_str)
+        .expect("parsing first century string should work");
+    assert_eq!(parsed, first_century);
+
+    let first_millenium = -59011459200;
+    let first_millenium_str = "0100-01-01T00:00:00Z";
+
+    let converted = epoch_to_rfc3339_utc(first_millenium)
+        .expect("converting epoch representing first millenium year should work");
+    assert_eq!(converted, first_millenium_str);
+
+    let parsed = parse_rfc3339(first_millenium_str)
+        .expect("parsing first millenium string should work");
+    assert_eq!(parsed, first_millenium);
+}
-- 
2.20.1

[pbs-devel] [PATCH proxmox 4/5] time/rfc3339: add leading zeroes for years < 1000

[Fabian Grünbichler]

strftime(3) does not mention this explicitly, but years before 1000 have
their leading zero(es) stripped, which is not valid according to either
ISO-8601 or its profile RFC3339.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 proxmox/src/tools/time.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox/src/tools/time.rs b/proxmox/src/tools/time.rs
index 5747854..3869757 100644
--- a/proxmox/src/tools/time.rs
+++ b/proxmox/src/tools/time.rs
@@ -168,7 +168,7 @@ pub fn epoch_to_rfc3339_utc(epoch: i64) -> Result<String, Error> {
         bail!("epoch_to_rfc3339_utc: wrong year '{}'", year);
     }
 
-    strftime("%FT%TZ", &gmtime)
+    strftime("%010FT%TZ", &gmtime)
 }
 
 /// Convert Unix epoch into RFC3339 local time with TZ
@@ -197,7 +197,7 @@ pub fn epoch_to_rfc3339(epoch: i64) -> Result<String, Error> {
     let hours = mins / 60;
     let mins = mins % 60;
 
-    let mut s = strftime("%FT%T", &localtime)?;
+    let mut s = strftime("%10FT%T", &localtime)?;
     s.push(prefix);
     s.push_str(&format!("{:02}:{:02}", hours, mins));
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox 5/5] time: add tests for gmtime range

[Fabian Grünbichler]

mainly so we notice if this assumption does not hold for some platform
or changes in the future.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 proxmox/src/tools/time.rs | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/proxmox/src/tools/time.rs b/proxmox/src/tools/time.rs
index 3869757..8527fe0 100644
--- a/proxmox/src/tools/time.rs
+++ b/proxmox/src/tools/time.rs
@@ -356,3 +356,26 @@ fn test_rfc3339_range() {
         .expect("parsing first millenium string should work");
     assert_eq!(parsed, first_millenium);
 }
+
+#[test]
+fn test_gmtime_range() {
+    // year must fit into i32
+    let lower = -67768040609740800;
+    let upper = 67768036191676799;
+
+    let mut lower_tm = gmtime(lower)
+        .expect("gmtime should work as long as years fit into i32");
+    let res = timegm(&mut lower_tm).expect("converting back to epoch should work");
+    assert_eq!(lower, res);
+
+    gmtime(lower-1)
+        .expect_err("gmtime should fail for years not fitting into i32");
+
+    let mut upper_tm = gmtime(upper)
+        .expect("gmtime should work as long as years fit into i32");
+    let res = timegm(&mut upper_tm).expect("converting back to epoch should work");
+    assert_eq!(upper, res);
+
+    gmtime(upper+1)
+        .expect_err("gmtime should fail for years not fitting into i32");
+}
-- 
2.20.1

Re: [pbs-devel] [RFC proxmox 1/5] time: add test for leap second parsing/converting

[Thomas Lamprecht]

On 9/15/20 12:20 PM, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     test fails, fixed by next patch
> 

off-topic, you and Dominik use this pattern to introduce test failure before
fixing it quite a bit, and I dislike it quite a bit.

Either just do the fix and the test in one patch, or re-order this so that
the fix comes first, then the previously failing test if you really think
it is semantically separate and needs to be a separate test...
We're all developers with at least a basic git experience, if one wants to
still check the prev. failure of the test anybody should be able to do that.

I do not want the build to fail at any commit.

[pbs-devel] applied: [PATCH proxmox 0/5] time conversion tests and fixes

[Dietmar Maurer]

applied

> On 09/15/2020 12:20 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> patches 1/2 could also be changed to not allow leap seconds when parsing
> at all - which would no longer be RFC3339 conformant, but we are not
> w.r.t some other things as well (e.g., t/z instead of T/Z).
> 
> Fabian Grünbichler (5):
>   time: add test for leap second parsing/converting
>   time: allow leap seconds when parsing RFC3339
>   time: add tests for RFC3339 corner cases
>   time/rfc3339: add leading zeroes for years < 1000
>   time: add tests for gmtime range
> 
>  proxmox/src/tools/time.rs | 106 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 103 insertions(+), 3 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel