[pbs-devel] [RFC PATCH v2 proxmox-backup-qemu 4/7] api: Supply `protected` parameter to the `finish` API call

[Christoph Heiss <c.heiss@proxmox.com>]

This can be used to set backups as protected as soon as the transfer is
finished, to e.g. avoid races while trying to set it protected later on,
while the PBS might have locked it already for verification.

! This is a breaking API/ABI change !

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Depends on the previous proxmox-backup patches (1-3 in the series).

RFC question: It would probably be sensible to issue a warning in
BackupTask::finish() - but what is the right way for that? A simple
eprintln!()?

Changes v1 -> v2:
* Use new server feature mechanism to detect support for API parameter

 current-api.h   |  3 ++-
 simpletest.c    |  2 +-
 src/backup.rs   | 24 ++++++++++++++++++++----
 src/commands.rs | 10 +++++++++-
 src/lib.rs      |  5 ++++-
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/current-api.h b/current-api.h
index f38ad4b..729a3a3 100644
--- a/current-api.h
+++ b/current-api.h
@@ -271,7 +271,7 @@ void proxmox_backup_close_image_async(struct ProxmoxBackupHandle *handle,
 /**
  * Finish the backup (sync)
  */
-int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, char **error);
+int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, bool protected_, char **error);

 /**
  * Finish the backup
@@ -280,6 +280,7 @@ int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, char **error);
  * All registered images have to be closed before calling this.
  */
 void proxmox_backup_finish_async(struct ProxmoxBackupHandle *handle,
+                                 bool protected_,
                                  void (*callback)(void*),
                                  void *callback_data,
                                  int *result,
diff --git a/simpletest.c b/simpletest.c
index ceb5afd..b061bde 100644
--- a/simpletest.c
+++ b/simpletest.c
@@ -77,7 +77,7 @@ void main(int argc, char **argv) {
   }

   printf("finish backup\n");
-  if (proxmox_backup_finish(pbs, &pbs_error) < 0) {
+  if (proxmox_backup_finish(pbs, false, &pbs_error) < 0) {
     fprintf(stderr, "proxmox_backup_finish failed - %s\n", pbs_error);
     proxmox_backup_free_error(pbs_error);
     exit(-1);
diff --git a/src/backup.rs b/src/backup.rs
index bbe4f00..60ec453 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -10,8 +10,10 @@ use tokio::runtime::Runtime;
 use proxmox_async::runtime::get_runtime_with_builder;
 use proxmox_sys::fs::file_get_contents;

-use pbs_api_types::{BackupType, CryptMode};
-use pbs_client::{BackupWriter, HttpClient, HttpClientOptions};
+use pbs_api_types::{BackupType, CryptMode, ServerFeature};
+use pbs_client::{
+    tools::get_supported_server_features, BackupWriter, HttpClient, HttpClientOptions,
+};
 use pbs_datastore::BackupManifest;
 use pbs_key_config::{load_and_decrypt_key, rsa_encrypt_key_config, KeyConfig};
 use pbs_tools::crypt_config::CryptConfig;
@@ -35,6 +37,7 @@ pub(crate) struct BackupTask {
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
     abort: tokio::sync::broadcast::Sender<()>,
     aborted: OnceCell<String>, // set on abort, conatins abort reason
+    server_features: OnceCell<Vec<ServerFeature>>,
 }

 impl BackupTask {
@@ -92,6 +95,7 @@ impl BackupTask {
             writer: OnceCell::new(),
             last_manifest: OnceCell::new(),
             aborted: OnceCell::new(),
+            server_features: OnceCell::new(),
         })
     }

@@ -141,10 +145,15 @@ impl BackupTask {
                 &self.setup.auth_id,
                 options,
             )?;
+
+            self.server_features
+                .set(get_supported_server_features(&http).await)
+                .map_err(|_| format_err!("already connected!"))?;
+
             let mut backup_dir = self.setup.backup_dir.clone();
             backup_dir.group.ty = BackupType::Vm;
             let writer = BackupWriter::start(
-                http,
+                &http,
                 self.crypt_config.clone(),
                 &self.setup.store,
                 &self.setup.backup_ns,
@@ -286,14 +295,21 @@ impl BackupTask {
         abortable_command(command_future, abort_rx.recv()).await
     }

-    pub async fn finish(&self) -> Result<c_int, Error> {
+    pub async fn finish(&self, protected: bool) -> Result<c_int, Error> {
         self.check_aborted()?;

+        let protected_supported = self
+            .server_features
+            .get()
+            .map(|sf| sf.contains(&ServerFeature::FinishHasProtectedParam))
+            .unwrap_or(false);
+
         let command_future = finish_backup(
             self.need_writer()?,
             self.crypt_config.clone(),
             self.rsa_encrypted_key.clone(),
             Arc::clone(&self.manifest),
+            protected && protected_supported,
         );

         let mut abort_rx = self.abort.subscribe();
diff --git a/src/commands.rs b/src/commands.rs
index 37d653c..4258711 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -468,11 +468,15 @@ pub(crate) async fn write_data(
     Ok(if reused { 0 } else { size as c_int })
 }

+/// The caller must ensure that all used server features are actually supported by the server.
+/// Currently, this only applies to `protected`. If this is set to true, the server has to support
+/// the `FinishHasProtectedParam` feature flag.
 pub(crate) async fn finish_backup(
     client: Arc<BackupWriter>,
     crypt_config: Option<Arc<CryptConfig>>,
     rsa_encrypted_key: Option<Vec<u8>>,
     manifest: Arc<Mutex<BackupManifest>>,
+    protected: bool,
 ) -> Result<c_int, Error> {
     if let Some(rsa_encrypted_key) = rsa_encrypted_key {
         let target = ENCRYPTED_KEY_BLOB_NAME;
@@ -521,7 +525,11 @@ pub(crate) async fn finish_backup(
         .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
         .await?;

-    client.finish().await?;
+    let mut param = json!({});
+    if protected {
+        param["protected"] = json!(true);
+    }

+    client.finish(Some(param)).await?;
     Ok(0)
 }
diff --git a/src/lib.rs b/src/lib.rs
index b3c7b85..2fe234b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -703,6 +703,7 @@ pub extern "C" fn proxmox_backup_close_image_async(
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub extern "C" fn proxmox_backup_finish(
     handle: *mut ProxmoxBackupHandle,
+    protected: bool,
     error: *mut *mut c_char,
 ) -> c_int {
     let mut result: c_int = -1;
@@ -713,6 +714,7 @@ pub extern "C" fn proxmox_backup_finish(

     proxmox_backup_finish_async(
         handle,
+        protected,
         callback_info.callback,
         callback_info.callback_data,
         callback_info.result,
@@ -732,6 +734,7 @@ pub extern "C" fn proxmox_backup_finish(
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub extern "C" fn proxmox_backup_finish_async(
     handle: *mut ProxmoxBackupHandle,
+    protected: bool,
     callback: extern "C" fn(*mut c_void),
     callback_data: *mut c_void,
     result: *mut c_int,
@@ -746,7 +749,7 @@ pub extern "C" fn proxmox_backup_finish_async(
     };

     task.runtime().spawn(async move {
-        let result = task.finish().await;
+        let result = task.finish(protected).await;
         callback_info.send_result(result);
     });
 }
--
2.34.1