vmm: add VmSendMigrationData::parse(); integrate with OptionParser

This change prepares upcoming options (following commit) that are added
to VmSendMigrationData.

VmSendMigrationData is a special case as it is currently the only
"rich configuration" type that lives outside `config.rs`, as it is
purely API-facing. Therefore, it isn't integrated into the existing
OptionParser infrastructure. We therefore introduce a `parse()` method
to use that in `ch-remote` in the following.

In `ch-remote`, we remove `--local` for `send-migration` and switch to
the new option string parsing constructor (breaking change!). This
prepares the addition of downtime and timeout options in the following
and streamlines the `ch-remote` command line interface with other
commands, such as `ch-remote add-net`.

Lastly, this commit updates the integration tests.

Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com

cloud-hypervisor/src/bin/ch-remote.rs

@@ -16,7 +16,9 @@ use api_client::{
     Error as ApiClientError, simple_api_command, simple_api_command_with_fds,
     simple_api_full_command,
 };
-use clap::{Arg, ArgAction, ArgMatches, Command};
+#[cfg(feature = "dbus_api")]
+use clap::ArgAction;
+use clap::{Arg, ArgMatches, Command};
 use log::error;
 use option_parser::{ByteSized, ByteSizedParseError};
 use thiserror::Error;
@@ -69,6 +71,8 @@ enum Error {
     ReadingFile(#[source] std::io::Error),
     #[error("Invalid disk size")]
     InvalidDiskSize(#[source] ByteSizedParseError),
+    #[error("Error parsing send migration configuration")]
+    SendMigrationConfig(#[from] vmm::api::VmSendMigrationParseError),
 }
 
 enum TargetApi<'a> {
@@ -519,11 +523,7 @@ fn rest_api_do_command(matches: &ArgMatches, socket: &mut UnixStream) -> ApiResu
                     .unwrap()
                     .get_one::<String>("send_migration_config")
                     .unwrap(),
-                matches
+            )?;
-                    .subcommand_matches("send-migration")
-                    .unwrap()
-                    .get_flag("send_migration_local"),
-            );
             simple_api_command(socket, "PUT", "send-migration", Some(&send_migration_data))
                 .map_err(Error::HttpApiClient)
         }
@@ -743,11 +743,7 @@ fn dbus_api_do_command(matches: &ArgMatches, proxy: &DBusApi1ProxyBlocking<'_>)
                     .unwrap()
                     .get_one::<String>("send_migration_config")
                     .unwrap(),
-                matches
+            )?;
-                    .subcommand_matches("send-migration")
-                    .unwrap()
-                    .get_flag("send_migration_local"),
-            );
             proxy.api_vm_send_migration(&send_migration_data)
         }
         Some("receive-migration") => {
@@ -953,13 +949,11 @@ fn receive_migration_data(url: &str) -> String {
     serde_json::to_string(&receive_migration_data).unwrap()
 }
 
-fn send_migration_data(url: &str, local: bool) -> String {
+fn send_migration_data(config: &str) -> Result<String, Error> {
-    let send_migration_data = vmm::api::VmSendMigrationData {
+    let send_migration_data =
-        destination_url: url.to_owned(),
+        vmm::api::VmSendMigrationData::parse(config).map_err(Error::SendMigrationConfig)?;
-        local,
+    let send_migration_config = serde_json::to_string(&send_migration_data).unwrap();
-    };
+    Ok(send_migration_config)
-
-    serde_json::to_string(&send_migration_data).unwrap()
 }
 
 fn create_data(path: &str) -> Result<String, Error> {
@@ -1141,13 +1135,7 @@ fn get_cli_commands_sorted() -> Box<[Command]> {
             .arg(
                 Arg::new("send_migration_config")
                     .index(1)
-                    .help("<destination_url>"),
+                    .help(vmm::api::VmSendMigrationData::SYNTAX),
-            )
-            .arg(
-                Arg::new("send_migration_local")
-                    .long("local")
-                    .num_args(0)
-                    .action(ArgAction::SetTrue),
             ),
         Command::new("shutdown").about("Shutdown the VM"),
         Command::new("shutdown-vmm").about("Shutdown the VMM"),

cloud-hypervisor/tests/integration.rs

@@ -9891,17 +9891,16 @@ mod live_migration {
         thread::sleep(std::time::Duration::new(1, 0));
         // Start to send migration from the source VM
 
-        let mut args = [
+        let args = [
             format!("--api-socket={}", &src_api_socket),
             "send-migration".to_string(),
-            format! {"unix:{migration_socket}"},
+            format!(
+                "destination_url=unix:{migration_socket},local={}",
+                if local { "on" } else { "off" }
+            ),
         ]
         .to_vec();
 
-        if local {
-            args.insert(2, "--local".to_string());
-        }
-
         let mut send_migration = Command::new(clh_command("ch-remote"))
             .args(&args)
             .stderr(Stdio::piped())
@@ -11066,7 +11065,7 @@ mod live_migration {
             .args([
                 &format!("--api-socket={src_api_socket}"),
                 "send-migration",
-                &format!("tcp:{host_ip}:{migration_port}"),
+                &format!("destination_url=tcp:{host_ip}:{migration_port}"),
             ])
             .stdin(Stdio::null())
             .stderr(Stdio::piped())

vmm/src/api/mod.rs

@@ -38,6 +38,7 @@ use std::sync::mpsc::{RecvError, SendError, Sender, channel};
 
 use log::info;
 use micro_http::Body;
+use option_parser::{OptionParser, OptionParserError, Toggle};
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 use vm_migration::MigratableError;
@@ -266,7 +267,12 @@ pub struct VmReceiveMigrationData {
     pub receiver_url: String,
 }
 
-#[derive(Clone, Deserialize, Serialize, Default, Debug)]
+#[derive(Debug, Error)]
+#[error("Error parsing send migration parameters")]
+pub struct VmSendMigrationParseError(#[source] OptionParserError);
+
+/// Configuration for an outgoing migration.
+#[derive(Clone, Deserialize, Serialize, Debug)]
 pub struct VmSendMigrationData {
     /// URL to migrate the VM to
     pub destination_url: String,
@@ -275,6 +281,33 @@ pub struct VmSendMigrationData {
     pub local: bool,
 }
 
+impl VmSendMigrationData {
+    pub const SYNTAX: &'static str = "VM send migration parameters \
+        \"destination_url=<url>[,local=on|off]\"";
+
+    pub fn parse(migration: &str) -> Result<Self, VmSendMigrationParseError> {
+        let mut parser = OptionParser::new();
+        parser.add("destination_url").add("local");
+        parser.parse(migration).map_err(VmSendMigrationParseError)?;
+
+        let destination_url = parser.get("destination_url").ok_or_else(|| {
+            VmSendMigrationParseError(OptionParserError::InvalidSyntax(
+                "destination_url is required".to_string(),
+            ))
+        })?;
+        let local = parser
+            .convert::<Toggle>("local")
+            .map_err(VmSendMigrationParseError)?
+            .unwrap_or(Toggle(false))
+            .0;
+
+        Ok(Self {
+            destination_url,
+            local,
+        })
+    }
+}
+
 pub enum ApiResponsePayload {
     /// No data is sent on the channel.
     Empty,
@@ -1541,3 +1574,23 @@ impl ApiAction for VmNmi {
         get_response_body(self, api_evt, api_sender, data)
     }
 }
+
+#[cfg(test)]
+mod unit_tests {
+    use super::*;
+
+    #[test]
+    fn test_vm_send_migration_data_parse() {
+        // Fully specified
+        let data = VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080,local=on")
+            .expect("valid migration string should parse");
+        assert_eq!(data.destination_url, "tcp://192.168.1.1:8080");
+        assert!(data.local);
+
+        // Unknown option is an error
+        VmSendMigrationData::parse("destination_url=unix:/tmp/sock,unknown_field=foo").unwrap_err();
+
+        // Invalid toggle value is an error
+        VmSendMigrationData::parse("destination_url=unix:/tmp/sock,local=yes").unwrap_err();
+    }
+}