[pbs-devel] [PATCH proxmox-backup(-qemu) 0/6] improve timestamp handling

[pbs-devel] [PATCH proxmox-backup(-qemu) 0/6] improve timestamp handling

[Fabian Grünbichler]

chrono provides two methods to convert seconds+nanoseconds to a
DateTime:

TimeZone::timestamp(), which panics on invalid input
TimeZone::timestamp_opt(), which returns a Result-type

this series changes most call sites to use the latter, especially when
epoch timestamps are transferred between client and server or are
provided by the user.

depending on context, either raising a nicer error, or using/printing
the original input values is done when encountering a value that chrono
does not handle.

[pbs-devel] [PATCH proxmox-backup 1/6] catalog dump: preserve original mtime

[Fabian Grünbichler]

even if it can't be handled by chrono. silently replacing it with epoch
0 is confusing..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/catalog.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backup/catalog.rs b/src/backup/catalog.rs
index bc2e471e..85a32623 100644
--- a/src/backup/catalog.rs
+++ b/src/backup/catalog.rs
@@ -5,7 +5,7 @@ use std::io::{Read, Write, Seek, SeekFrom};
 use std::os::unix::ffi::OsStrExt;
 
 use anyhow::{bail, format_err, Error};
-use chrono::offset::{TimeZone, Local};
+use chrono::offset::{TimeZone, Local, LocalResult};
 
 use pathpatterns::{MatchList, MatchType};
 use proxmox::tools::io::ReadExt;
@@ -533,17 +533,17 @@ impl <R: Read + Seek> CatalogReader<R> {
                     self.dump_dir(&path, pos)?;
                 }
                 CatalogEntryType::File => {
-                    let dt = Local
-                        .timestamp_opt(mtime as i64, 0)
-                        .single() // chrono docs say timestamp_opt can only be None or Single!
-                        .unwrap_or_else(|| Local.timestamp(0, 0));
+                    let mtime_string = match Local.timestamp_opt(mtime as i64, 0) {
+                        LocalResult::Single(time) => time.to_rfc3339_opts(chrono::SecondsFormat::Secs, false),
+                        _ => (mtime as i64).to_string(),
+                    };
 
                     println!(
                         "{} {:?} {} {}",
                         etype,
                         path,
                         size,
-                        dt.to_rfc3339_opts(chrono::SecondsFormat::Secs, false),
+                        mtime_string,
                     );
                 }
                 _ => {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/6] don't truncate DateTime nanoseconds

[Fabian Grünbichler]

where we don't care about them anyway..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Note: this does not change visible behaviour AFAICT, but makes the code
nicer to read IMHO..

 src/backup/crypt_config.rs                 | 4 ++--
 src/backup/key_derivation.rs               | 4 ++--
 src/bin/proxmox_backup_client/benchmark.rs | 4 ++--
 src/bin/proxmox_backup_client/key.rs       | 6 +++---
 src/tools/file_logger.rs                   | 4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs
index 72b56dd7..c30fb5fd 100644
--- a/src/backup/crypt_config.rs
+++ b/src/backup/crypt_config.rs
@@ -10,7 +10,7 @@
 use std::io::Write;
 
 use anyhow::{bail, Error};
-use chrono::{Local, TimeZone, DateTime};
+use chrono::{Local, DateTime};
 use openssl::hash::MessageDigest;
 use openssl::pkcs5::pbkdf2_hmac;
 use openssl::symm::{decrypt_aead, Cipher, Crypter, Mode};
@@ -219,7 +219,7 @@ impl CryptConfig {
         created: DateTime<Local>,
     ) -> Result<Vec<u8>, Error> {
 
-        let modified = Local.timestamp(Local::now().timestamp(), 0);
+        let modified = Local::now();
         let key_config = super::KeyConfig { kdf: None, created, modified, data: self.enc_key.to_vec() };
         let data = serde_json::to_string(&key_config)?.as_bytes().to_vec();
 
diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 4befe525..ac27da63 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -1,7 +1,7 @@
 use anyhow::{bail, format_err, Context, Error};
 
 use serde::{Deserialize, Serialize};
-use chrono::{Local, TimeZone, DateTime};
+use chrono::{Local, DateTime};
 
 use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions};
 use proxmox::try_block;
@@ -136,7 +136,7 @@ pub fn encrypt_key_with_passphrase(
     enc_data.extend_from_slice(&tag);
     enc_data.extend_from_slice(&encrypted_key);
 
-    let created =  Local.timestamp(Local::now().timestamp(), 0);
+    let created = Local::now();
 
     Ok(KeyConfig {
         kdf: Some(kdf),
diff --git a/src/bin/proxmox_backup_client/benchmark.rs b/src/bin/proxmox_backup_client/benchmark.rs
index 159b0c18..9830c183 100644
--- a/src/bin/proxmox_backup_client/benchmark.rs
+++ b/src/bin/proxmox_backup_client/benchmark.rs
@@ -3,7 +3,7 @@ use std::sync::Arc;
 
 use anyhow::{Error};
 use serde_json::Value;
-use chrono::{TimeZone, Utc};
+use chrono::Utc;
 use serde::Serialize;
 
 use proxmox::api::{ApiMethod, RpcEnvironment};
@@ -212,7 +212,7 @@ async fn test_upload_speed(
     verbose: bool,
 ) -> Result<(), Error> {
 
-    let backup_time = Utc.timestamp(Utc::now().timestamp(), 0);
+    let backup_time = Utc::now();
 
     let client = connect(repo.host(), repo.user())?;
     record_repository(&repo);
diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index f85e80c7..30afa4e5 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -1,7 +1,7 @@
 use std::path::PathBuf;
 
 use anyhow::{bail, format_err, Error};
-use chrono::{Local, TimeZone};
+use chrono::Local;
 use serde::{Deserialize, Serialize};
 
 use proxmox::api::api;
@@ -112,7 +112,7 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
 
     match kdf {
         Kdf::None => {
-            let created = Local.timestamp(Local::now().timestamp(), 0);
+            let created = Local::now();
 
             store_key_config(
                 &path,
@@ -180,7 +180,7 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
 
     match kdf {
         Kdf::None => {
-            let modified = Local.timestamp(Local::now().timestamp(), 0);
+            let modified = Local::now();
 
             store_key_config(
                 &path,
diff --git a/src/tools/file_logger.rs b/src/tools/file_logger.rs
index 426e7b8d..c0fcab78 100644
--- a/src/tools/file_logger.rs
+++ b/src/tools/file_logger.rs
@@ -1,5 +1,5 @@
 use anyhow::{Error};
-use chrono::{TimeZone, Local};
+use chrono::Local;
 use std::io::Write;
 
 /// Log messages with timestamps into files
@@ -56,7 +56,7 @@ impl FileLogger {
             stdout.write_all(b"\n").unwrap();
         }
 
-        let line = format!("{}: {}\n", Local.timestamp(Local::now().timestamp(), 0).to_rfc3339(), msg);
+        let line = format!("{}: {}\n", Local::now().to_rfc3339(), msg);
         self.file.write_all(line.as_bytes()).unwrap();
     }
 }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/6] use non-panicky timestamp_opt where appropriate

[Fabian Grünbichler]

by either printing the original, out-of-range timestamp as-is, or
bailing with a proper error message instead of panicking.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/fixed_index.rs        |  7 +++++--
 src/bin/proxmox-backup-client.rs | 17 ++++++++++++++---
 src/tools/format.rs              |  9 ++++++---
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/backup/fixed_index.rs b/src/backup/fixed_index.rs
index 5d6cc1ff..309f4503 100644
--- a/src/backup/fixed_index.rs
+++ b/src/backup/fixed_index.rs
@@ -6,7 +6,7 @@ use super::chunk_store::*;
 use super::{IndexFile, ChunkReadInfo};
 use crate::tools::{self, epoch_now_u64};
 
-use chrono::{Local, TimeZone};
+use chrono::{Local, LocalResult, TimeZone};
 use std::fs::File;
 use std::io::Write;
 use std::os::unix::io::AsRawFd;
@@ -150,7 +150,10 @@ impl FixedIndexReader {
         println!("ChunkSize: {}", self.chunk_size);
         println!(
             "CTime: {}",
-            Local.timestamp(self.ctime as i64, 0).format("%c")
+            match Local.timestamp_opt(self.ctime as i64, 0) {
+                LocalResult::Single(ctime) => ctime.format("%c").to_string(),
+                _ => (self.ctime as i64).to_string(),
+            }
         );
         println!("UUID: {:?}", self.uuid);
     }
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 1e9cc680..19f8ccda 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -8,7 +8,7 @@ use std::sync::{Arc, Mutex};
 use std::task::Context;
 
 use anyhow::{bail, format_err, Error};
-use chrono::{Local, DateTime, Utc, TimeZone};
+use chrono::{Local, LocalResult, DateTime, Utc, TimeZone};
 use futures::future::FutureExt;
 use futures::stream::{StreamExt, TryStreamExt};
 use serde_json::{json, Value};
@@ -257,7 +257,11 @@ pub async fn api_datastore_latest_snapshot(
 
     list.sort_unstable_by(|a, b| b.backup_time.cmp(&a.backup_time));
 
-    let backup_time = Utc.timestamp(list[0].backup_time, 0);
+    let backup_time = match Utc.timestamp_opt(list[0].backup_time, 0) {
+        LocalResult::Single(time) => time,
+        _ => bail!("last snapshot of backup group {:?} has invalid timestmap {}.",
+                   group.group_path(), list[0].backup_time),
+    };
 
     Ok((group.backup_type().to_owned(), group.backup_id().to_owned(), backup_time))
 }
@@ -986,7 +989,15 @@ async fn create_backup(
         }
     }
 
-    let backup_time = Utc.timestamp(backup_time_opt.unwrap_or_else(|| Utc::now().timestamp()), 0);
+    let backup_time = match backup_time_opt {
+        Some(timestamp) => {
+            match Utc.timestamp_opt(timestamp, 0) {
+                LocalResult::Single(time) => time,
+                _ => bail!("Invalid backup-time parameter: {}", timestamp),
+            }
+        },
+        _ => Utc::now(),
+    };
 
     let client = connect(repo.host(), repo.user())?;
     record_repository(&repo);
diff --git a/src/tools/format.rs b/src/tools/format.rs
index aa67ea38..1e593ff3 100644
--- a/src/tools/format.rs
+++ b/src/tools/format.rs
@@ -1,6 +1,6 @@
 use anyhow::{Error};
 use serde_json::Value;
-use chrono::{Local, TimeZone};
+use chrono::{Local, TimeZone, LocalResult};
 
 pub fn strip_server_file_expenstion(name: &str) -> String {
 
@@ -25,8 +25,11 @@ pub fn render_epoch(value: &Value, _record: &Value) -> Result<String, Error> {
     if value.is_null() { return Ok(String::new()); }
     let text = match value.as_i64() {
         Some(epoch) => {
-            Local.timestamp(epoch, 0).format("%c").to_string()
-        }
+            match Local.timestamp_opt(epoch, 0) {
+                LocalResult::Single(epoch) => epoch.format("%c").to_string(),
+                _ => epoch.to_string(),
+            }
+        },
         None => {
             value.to_string()
         }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/6] handle invalid mtime when formating entries

[Fabian Grünbichler]

otherwise operations like catalog shell panic when viewing pxar archives
containing such entries, e.g. with mtime very far ahead into the future.

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

diff --git a/src/pxar/tools.rs b/src/pxar/tools.rs
index 0fdb033d..2eed0f81 100644
--- a/src/pxar/tools.rs
+++ b/src/pxar/tools.rs
@@ -8,7 +8,7 @@ use std::path::Path;
 use anyhow::{bail, format_err, Error};
 use nix::sys::stat::Mode;
 
-use pxar::{mode, Entry, EntryKind, Metadata};
+use pxar::{mode, Entry, EntryKind, Metadata, format::StatxTimestamp};
 
 /// Get the file permissions as `nix::Mode`
 pub fn perms_from_metadata(meta: &Metadata) -> Result<Mode, Error> {
@@ -114,13 +114,19 @@ fn mode_string(entry: &Entry) -> String {
     )
 }
 
-pub fn format_single_line_entry(entry: &Entry) -> String {
+fn format_mtime(mtime: &StatxTimestamp) -> String {
     use chrono::offset::TimeZone;
 
+    match chrono::Local.timestamp_opt(mtime.secs, mtime.nanos) {
+        chrono::LocalResult::Single(mtime) => mtime.format("%Y-%m-%d %H:%M:%S").to_string(),
+        _ => format!("{}.{}", mtime.secs, mtime.nanos),
+    }
+}
+
+pub fn format_single_line_entry(entry: &Entry) -> String {
     let mode_string = mode_string(entry);
 
     let meta = entry.metadata();
-    let mtime = chrono::Local.timestamp(meta.stat.mtime.secs, meta.stat.mtime.nanos);
 
     let (size, link) = match entry.kind() {
         EntryKind::File { size, .. } => (format!("{}", *size), String::new()),
@@ -134,7 +140,7 @@ pub fn format_single_line_entry(entry: &Entry) -> String {
         "{} {:<13} {} {:>8} {:?}{}",
         mode_string,
         format!("{}/{}", meta.stat.uid, meta.stat.gid),
-        mtime.format("%Y-%m-%d %H:%M:%S"),
+        format_mtime(&meta.stat.mtime),
         size,
         entry.path(),
         link,
@@ -142,12 +148,9 @@ pub fn format_single_line_entry(entry: &Entry) -> String {
 }
 
 pub fn format_multi_line_entry(entry: &Entry) -> String {
-    use chrono::offset::TimeZone;
-
     let mode_string = mode_string(entry);
 
     let meta = entry.metadata();
-    let mtime = chrono::Local.timestamp(meta.stat.mtime.secs, meta.stat.mtime.nanos);
 
     let (size, link, type_name) = match entry.kind() {
         EntryKind::File { size, .. } => (format!("{}", *size), String::new(), "file"),
@@ -196,6 +199,6 @@ pub fn format_multi_line_entry(entry: &Entry) -> String {
         mode_string,
         meta.stat.uid,
         meta.stat.gid,
-        mtime.format("%Y-%m-%d %H:%M:%S"),
+        format_mtime(&meta.stat.mtime),
     )
 }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/6] BackupDir: make constructor fallible

[Fabian Grünbichler]

since converting from i64 epoch timestamp to DateTime is not always
possible. previously, passing invalid backup-time from client to server
(or vice-versa) panicked the corresponding tokio task. now we get proper
error messages including the invalid timestamp.

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

Notes:
    needs corresponding patch in proxmox-backup-qemu.
    
    maybe we also want to provide constructors that take a DateTime,
    there are a few callers that already have one and convert it to an
    i64 to create a BackupDir that immediately converts it back to a
    DateTime again..

 src/api2/admin/datastore.rs      | 20 +++++++++---------
 src/api2/backup.rs               |  2 +-
 src/api2/reader.rs               |  2 +-
 src/backup/backup_info.rs        | 36 +++++++++++++++++++-------------
 src/bin/proxmox-backup-client.rs | 13 ++++++------
 src/client/pull.rs               |  2 +-
 6 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 85c84df4..be2796db 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -230,7 +230,7 @@ pub fn list_snapshot_files(
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
-    let snapshot = BackupDir::new(backup_type, backup_id, backup_time);
+    let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)) != 0;
     if !allowed { check_backup_owner(&datastore, snapshot.group(), &userid)?; }
@@ -280,7 +280,7 @@ fn delete_snapshot(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
 
-    let snapshot = BackupDir::new(backup_type, backup_id, backup_time);
+    let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
@@ -490,7 +490,7 @@ pub fn verify(
     match (backup_type, backup_id, backup_time) {
         (Some(backup_type), Some(backup_id), Some(backup_time)) => {
             worker_id = format!("{}_{}_{}_{:08X}", store, backup_type, backup_id, backup_time);
-            let dir = BackupDir::new(backup_type, backup_id, backup_time);
+            let dir = BackupDir::new(backup_type, backup_id, backup_time)?;
             backup_dir = Some(dir);
         }
         (Some(backup_type), Some(backup_id), None) => {
@@ -897,7 +897,7 @@ fn download_file(
         let backup_id = tools::required_string_param(&param, "backup-id")?;
         let backup_time = tools::required_integer_param(&param, "backup-time")?;
 
-        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
         let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
         if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
@@ -970,7 +970,7 @@ fn download_file_decoded(
         let backup_id = tools::required_string_param(&param, "backup-id")?;
         let backup_time = tools::required_integer_param(&param, "backup-time")?;
 
-        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
         let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
         if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
@@ -1083,7 +1083,7 @@ fn upload_backup_log(
         let backup_id = tools::required_string_param(&param, "backup-id")?;
         let backup_time = tools::required_integer_param(&param, "backup-time")?;
 
-        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
         let userid: Userid = rpcenv.get_user().unwrap().parse()?;
         check_backup_owner(&datastore, backup_dir.group(), &userid)?;
@@ -1159,7 +1159,7 @@ fn catalog(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
 
-    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
     if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
@@ -1276,7 +1276,7 @@ fn pxar_file_download(
         let backup_id = tools::required_string_param(&param, "backup-id")?;
         let backup_time = tools::required_integer_param(&param, "backup-time")?;
 
-        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
         let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
         if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
@@ -1417,7 +1417,7 @@ fn get_notes(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
 
-    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
     if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
@@ -1470,7 +1470,7 @@ fn set_notes(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
 
-    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     let allowed = (user_privs & PRIV_DATASTORE_READ) != 0;
     if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; }
diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index c00f9be8..9420b146 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -114,7 +114,7 @@ async move {
     }
 
     let last_backup = BackupInfo::last_backup(&datastore.base_path(), &backup_group, true).unwrap_or(None);
-    let backup_dir = BackupDir::new_with_group(backup_group.clone(), backup_time);
+    let backup_dir = BackupDir::new_with_group(backup_group.clone(), backup_time)?;
 
     let _last_guard = if let Some(last) = &last_backup {
         if backup_dir.backup_time() <= last.backup_dir.backup_time() {
diff --git a/src/api2/reader.rs b/src/api2/reader.rs
index cf82af06..5252d2e9 100644
--- a/src/api2/reader.rs
+++ b/src/api2/reader.rs
@@ -83,7 +83,7 @@ fn upgrade_to_backup_reader_protocol(
 
         let env_type = rpcenv.env_type();
 
-        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time);
+        let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
         let path = datastore.base_path();
 
         //let files = BackupInfo::list_files(&path, &backup_dir)?;
diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs
index 4dcf897a..023625f5 100644
--- a/src/backup/backup_info.rs
+++ b/src/backup/backup_info.rs
@@ -2,9 +2,10 @@ use crate::tools;
 
 use anyhow::{bail, format_err, Error};
 use regex::Regex;
+use std::convert::TryFrom;
 use std::os::unix::io::RawFd;
 
-use chrono::{DateTime, TimeZone, SecondsFormat, Utc};
+use chrono::{DateTime, LocalResult, TimeZone, SecondsFormat, Utc};
 
 use std::path::{PathBuf, Path};
 use lazy_static::lazy_static;
@@ -106,7 +107,7 @@ impl BackupGroup {
             if file_type != nix::dir::Type::Directory { return Ok(()); }
 
             let dt = backup_time.parse::<DateTime<Utc>>()?;
-            let backup_dir = BackupDir::new(self.backup_type.clone(), self.backup_id.clone(), dt.timestamp());
+            let backup_dir = BackupDir::new(self.backup_type.clone(), self.backup_id.clone(), dt.timestamp())?;
             let files = list_backup_files(l2_fd, backup_time)?;
 
             list.push(BackupInfo { backup_dir, files });
@@ -208,19 +209,22 @@ pub struct BackupDir {
 
 impl BackupDir {
 
-    pub fn new<T, U>(backup_type: T, backup_id: U, timestamp: i64) -> Self
+    pub fn new<T, U>(backup_type: T, backup_id: U, timestamp: i64) -> Result<Self, Error>
     where
         T: Into<String>,
         U: Into<String>,
     {
-        // Note: makes sure that nanoseconds is 0
-        Self {
-            group: BackupGroup::new(backup_type.into(), backup_id.into()),
-            backup_time: Utc.timestamp(timestamp, 0),
-        }
+        let group = BackupGroup::new(backup_type.into(), backup_id.into());
+        BackupDir::new_with_group(group, timestamp)
     }
-    pub fn new_with_group(group: BackupGroup, timestamp: i64) -> Self {
-        Self { group, backup_time: Utc.timestamp(timestamp, 0) }
+
+    pub fn new_with_group(group: BackupGroup, timestamp: i64) -> Result<Self, Error> {
+        let backup_time = match Utc.timestamp_opt(timestamp, 0) {
+            LocalResult::Single(time) => time,
+            _ => bail!("can't create BackupDir with invalid backup time {}", timestamp),
+        };
+
+        Ok(Self { group, backup_time })
     }
 
     pub fn group(&self) -> &BackupGroup {
@@ -257,7 +261,7 @@ impl std::str::FromStr for BackupDir {
 
         let group = BackupGroup::new(cap.get(1).unwrap().as_str(), cap.get(2).unwrap().as_str());
         let backup_time = cap.get(3).unwrap().as_str().parse::<DateTime<Utc>>()?;
-        Ok(BackupDir::from((group, backup_time.timestamp())))
+        BackupDir::try_from((group, backup_time.timestamp()))
     }
 }
 
@@ -270,9 +274,11 @@ impl std::fmt::Display for BackupDir {
     }
 }
 
-impl From<(BackupGroup, i64)> for BackupDir {
-    fn from((group, timestamp): (BackupGroup, i64)) -> Self {
-        Self { group, backup_time: Utc.timestamp(timestamp, 0) }
+impl TryFrom<(BackupGroup, i64)> for BackupDir {
+    type Error = Error;
+
+    fn try_from((group, timestamp): (BackupGroup, i64)) -> Result<Self, Error> {
+        BackupDir::new_with_group(group, timestamp)
     }
 }
 
@@ -334,7 +340,7 @@ impl BackupInfo {
                     if file_type != nix::dir::Type::Directory { return Ok(()); }
 
                     let dt = backup_time.parse::<DateTime<Utc>>()?;
-                    let backup_dir = BackupDir::new(backup_type, backup_id, dt.timestamp());
+                    let backup_dir = BackupDir::new(backup_type, backup_id, dt.timestamp())?;
 
                     let files = list_backup_files(l2_fd, backup_time)?;
 
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 19f8ccda..aea715aa 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -376,7 +376,7 @@ async fn list_backup_groups(param: Value) -> Result<Value, Error> {
 
     let render_last_backup = |_v: &Value, record: &Value| -> Result<String, Error> {
         let item: GroupListItem = serde_json::from_value(record.to_owned())?;
-        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.last_backup);
+        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.last_backup)?;
         Ok(snapshot.relative_path().to_str().unwrap().to_owned())
     };
 
@@ -447,7 +447,7 @@ async fn list_snapshots(param: Value) -> Result<Value, Error> {
 
     let render_snapshot_path = |_v: &Value, record: &Value| -> Result<String, Error> {
         let item: SnapshotListItem = serde_json::from_value(record.to_owned())?;
-        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time);
+        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
         Ok(snapshot.relative_path().to_str().unwrap().to_owned())
     };
 
@@ -1046,7 +1046,7 @@ async fn create_backup(
         None
     };
 
-    let snapshot = BackupDir::new(backup_type, backup_id, backup_time.timestamp());
+    let snapshot = BackupDir::new(backup_type, backup_id, backup_time.timestamp())?;
     let mut manifest = BackupManifest::new(snapshot);
 
     let mut catalog = None;
@@ -1571,7 +1571,7 @@ async fn prune_async(mut param: Value) -> Result<Value, Error> {
 
     let render_snapshot_path = |_v: &Value, record: &Value| -> Result<String, Error> {
         let item: PruneListItem = serde_json::from_value(record.to_owned())?;
-        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time);
+        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
         Ok(snapshot.relative_path().to_str().unwrap().to_owned())
     };
 
@@ -1763,8 +1763,9 @@ async fn complete_backup_snapshot_do(param: &HashMap<String, String>) -> Vec<Str
             if let (Some(backup_id), Some(backup_type), Some(backup_time)) =
                 (item["backup-id"].as_str(), item["backup-type"].as_str(), item["backup-time"].as_i64())
             {
-                let snapshot = BackupDir::new(backup_type, backup_id, backup_time);
-                result.push(snapshot.relative_path().to_str().unwrap().to_owned());
+                if let Ok(snapshot) = BackupDir::new(backup_type, backup_id, backup_time) {
+                    result.push(snapshot.relative_path().to_str().unwrap().to_owned());
+                }
             }
         }
     }
diff --git a/src/client/pull.rs b/src/client/pull.rs
index 2428051a..ab7e9891 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -347,7 +347,7 @@ pub async fn pull_group(
     let mut remote_snapshots = std::collections::HashSet::new();
 
     for item in list {
-        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time);
+        let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
 
         // in-progress backups can't be synced
         if let None = item.size {
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 5/6] BackupDir: make constructor fallible

[Dietmar Maurer]

> since converting from i64 epoch timestamp to DateTime is not always
> possible. 

In our case we use UTC, and nanosecs is 0. So when will that fail?

Re: [pbs-devel] [PATCH proxmox-backup 5/6] BackupDir: make constructor fallible

[Dietmar Maurer]

> On 09/11/2020 4:12 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > since converting from i64 epoch timestamp to DateTime is not always
> > possible. 
> 
> In our case we use UTC, and nanosecs is 0. So when will that fail?

The chrono source code contains the following comment:

  /// Converts the UTC `NaiveDateTime` to the local time.
  /// The UTC is continuous and thus this cannot fail (but can give the duplicate local time).
  fn from_utc_datetime(&self, utc: &NaiveDateTime) -> DateTime<Self> {


So what does that mean?

[pbs-devel] [PATCH proxmox-backup-qemu 6/6] update to new BackupDir constructor

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 Cargo.toml    | 4 ++--
 src/backup.rs | 2 +-
 src/lib.rs    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 649b4c5..664bba2 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -25,7 +25,7 @@ libc = "0.2"
 once_cell = "1.3.1"
 openssl = "0.10"
 proxmox = { version = "0.3.5", features = [ "sortable-macro", "api-macro" ] }
-proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.8.15" }
-#proxmox-backup = { path = "../proxmox-backup" }
+#proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.8.15" }
+proxmox-backup = { path = "../proxmox-backup" }
 serde_json = "1.0"
 tokio = { version = "0.2.9", features = [ "blocking", "fs", "io-util", "macros", "rt-threaded", "signal", "stream", "tcp", "time", "uds" ] }
diff --git a/src/backup.rs b/src/backup.rs
index 094f8b3..aa65239 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -59,7 +59,7 @@ impl BackupTask {
 
         let (abort, _) = tokio::sync::broadcast::channel(16);
 
-        let snapshot = BackupDir::new(&setup.backup_type, &setup.backup_id, setup.backup_time.timestamp());
+        let snapshot = BackupDir::new(&setup.backup_type, &setup.backup_id, setup.backup_time.timestamp())?;
         let manifest = Arc::new(Mutex::new(BackupManifest::new(snapshot)));
 
         let registry = Arc::new(Mutex::new(Registry::<ImageUploadInfo>::new()));
diff --git a/src/lib.rs b/src/lib.rs
index 5bb3685..67f2fe9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -98,7 +98,7 @@ pub extern "C" fn proxmox_backup_snapshot_string(
         let backup_id: String = tools::utf8_c_string_lossy(backup_id)
             .ok_or_else(|| format_err!("backup_id must not be NULL"))?;
 
-        let snapshot = BackupDir::new(backup_type, backup_id, backup_time);
+        let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
 
         Ok(CString::new(format!("{}", snapshot))?)
     });
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup-qemu 6/6] update to new BackupDir constructor

[Dietmar Maurer]

applied (after proxmox-backup version bump)

> On 09/11/2020 2:34 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  Cargo.toml    | 4 ++--
>  src/backup.rs | 2 +-
>  src/lib.rs    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 649b4c5..664bba2 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -25,7 +25,7 @@ libc = "0.2"
>  once_cell = "1.3.1"
>  openssl = "0.10"
>  proxmox = { version = "0.3.5", features = [ "sortable-macro", "api-macro" ] }
> -proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.8.15" }
> -#proxmox-backup = { path = "../proxmox-backup" }
> +#proxmox-backup = { git = "git://git.proxmox.com/git/proxmox-backup.git", tag = "v0.8.15" }
> +proxmox-backup = { path = "../proxmox-backup" }
>  serde_json = "1.0"
>  tokio = { version = "0.2.9", features = [ "blocking", "fs", "io-util", "macros", "rt-threaded", "signal", "stream", "tcp", "time", "uds" ] }
> diff --git a/src/backup.rs b/src/backup.rs
> index 094f8b3..aa65239 100644
> --- a/src/backup.rs
> +++ b/src/backup.rs
> @@ -59,7 +59,7 @@ impl BackupTask {
>  
>          let (abort, _) = tokio::sync::broadcast::channel(16);
>  
> -        let snapshot = BackupDir::new(&setup.backup_type, &setup.backup_id, setup.backup_time.timestamp());
> +        let snapshot = BackupDir::new(&setup.backup_type, &setup.backup_id, setup.backup_time.timestamp())?;
>          let manifest = Arc::new(Mutex::new(BackupManifest::new(snapshot)));
>  
>          let registry = Arc::new(Mutex::new(Registry::<ImageUploadInfo>::new()));
> diff --git a/src/lib.rs b/src/lib.rs
> index 5bb3685..67f2fe9 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -98,7 +98,7 @@ pub extern "C" fn proxmox_backup_snapshot_string(
>          let backup_id: String = tools::utf8_c_string_lossy(backup_id)
>              .ok_or_else(|| format_err!("backup_id must not be NULL"))?;
>  
> -        let snapshot = BackupDir::new(backup_type, backup_id, backup_time);
> +        let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
>  
>          Ok(CString::new(format!("{}", snapshot))?)
>      });
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox-backup(-qemu) 0/6] improve timestamp handling

[Dietmar Maurer]

applied (but, some question will follow)

> On 09/11/2020 2:34 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> chrono provides two methods to convert seconds+nanoseconds to a
> DateTime:
> 
> TimeZone::timestamp(), which panics on invalid input
> TimeZone::timestamp_opt(), which returns a Result-type
> 
> this series changes most call sites to use the latter, especially when
> epoch timestamps are transferred between client and server or are
> provided by the user.
> 
> depending on context, either raising a nicer error, or using/printing
> the original input values is done when encountering a value that chrono
> does not handle.
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel