On 1/15/24 19:24, Richard W.M. Jones wrote:
Xml.uri is a convenient way to pass the multiple ssh fields to virt-v2v and is still used internally by the -i vmx code. However don't leak this awkward implementation into the new ssh.ml module.
Like Nbdkit_ssh, we'll only deal with the explicit (port, server, user, path) fields separately.
Note after this refactoring:
- The new Ssh module interface looks broadly similar to the Nbdkit_ssh interface.
- vmx_source_of_arg assertions are no longer required inside the new Ssh module, which was a mild layering violation before. --- input/ssh.mli | 21 ++++++++++----------- input/input_vmx.ml | 21 ++++++++++++++++----- input/parse_domain_from_vmx.ml | 20 ++++++++++++++++++-- input/ssh.ml | 32 +++++++++++--------------------- 4 files changed, 55 insertions(+), 39 deletions(-)
Complicated patch; I'll quote it out of (formatting) order:
diff --git a/input/ssh.mli b/input/ssh.mli index 62e78bd3e8..1f39cd1bea 100644 --- a/input/ssh.mli +++ b/input/ssh.mli @@ -18,16 +18,15 @@
(** Wrappers for finding and downloading remote files over ssh. *)
-val path_of_uri : Xml.uri -> string -val server_of_uri : Xml.uri -> string -val port_of_uri : Xml.uri -> int option +(** [remote_file_exists password ?port server ?user path] + checks that [path] exists on the remote server. *) +val remote_file_exists : password:Nbdkit_ssh.password -> + ?port:string -> server:string -> ?user:string -> + string -> bool
-(** [remote_file_exists ssh_uri path] checks that [path] exists - on the remote server [ssh_uri] (note any path inside [ssh_uri] - is ignored). *) -val remote_file_exists : Xml.uri -> string -> bool - -(** [download_file ssh_uri output] - uses scp to copy the single remote file at [ssh_uri] to +(** [download_file password ?port server ?user path output] + uses scp to copy the single remote file at [path] to the local file called [output]. *) -val download_file : Xml.uri -> string -> unit +val download_file : password:Nbdkit_ssh.password -> + ?port:string -> server:string -> ?user:string -> string -> + string -> unit
OK
diff --git a/input/ssh.ml b/input/ssh.ml index fbc0e54f5d..4e0e420e20 100644 --- a/input/ssh.ml +++ b/input/ssh.ml @@ -22,29 +22,19 @@ open Common_gettext.Gettext
open Printf
-(* Return various fields from the URI. The checks in vmx_source_of_arg - * should ensure that none of these assertions fail. - *) -let port_of_uri { Xml.uri_port } = - match uri_port with i when i <= 0 -> None | i -> Some i -let server_of_uri { Xml.uri_server } = - match uri_server with None -> assert false | Some s -> s -let path_of_uri { Xml.uri_path } = - match uri_path with None -> assert false | Some p -> p - (* 'scp' a remote file into a local file. *)
OK
-let download_file uri output = +let download_file ~password ?port ~server ?user path output =
(1) so here's where I suggest that (if syntactically possible) we don't bind "password", just use "_".
let cmd = sprintf "scp%s%s %s%s:%s %s" (if verbose () then "" else " -q") - (match port_of_uri uri with + (match port with | None -> "" - | Some port -> sprintf " -P %d" port) - (match uri.Xml.uri_user with + | Some port -> sprintf " -P %s" port) + (match user with | None -> "" | Some user -> quote user ^ "@") - (quote (server_of_uri uri)) - (quote (path_of_uri uri)) + (quote server) + (quote path) (quote output) in if verbose () then eprintf "%s\n%!" cmd;
OK (sneaky change: port changes from integer to string; but OK)
@@ -53,16 +43,16 @@ let download_file uri output = see earlier error messages")
(* Test if [path] exists on the remote server. *) -let remote_file_exists uri path = +let remote_file_exists ~password ?port ~server ?user path = let cmd = sprintf "ssh%s %s%s test -f %s" - (match port_of_uri uri with + (match port with | None -> "" - | Some port -> sprintf " -p %d" port) - (match uri.Xml.uri_user with + | Some port -> sprintf " -p %s" port) + (match user with | None -> "" | Some user -> quote user ^ "@") - (quote (server_of_uri uri)) + (quote server) (* Double quoting is necessary for 'ssh', first to protect * from the local shell, second to protect from the remote * shell. https://github.com/libguestfs/virt-v2v/issues/35#issuecomment-1741730963
Same two comments: (2) suggest binding the password with just "_" for now, and same "sneaky" (but otherwise harmless / intended) remark about the port type change.
diff --git a/input/input_vmx.ml b/input/input_vmx.ml index b9cce10fed..b3426fa242 100644 --- a/input/input_vmx.ml +++ b/input/input_vmx.ml @@ -83,22 +83,33 @@ module VMX = struct let socket = sprintf "%s/in%d" dir i in On_exit.unlink socket;
- let vmx_path = Ssh.path_of_uri uri in + let vmx_path = + match uri.uri_path with + | None -> assert false (* checked by vmx_source_of_arg *) + | Some path -> path in let abs_path = absolute_path_from_other_file vmx_path filename in let flat_vmdk = PCRE.replace (PCRE.compile "\\.vmdk$") "-flat.vmdk" abs_path in
+ let server = + match uri.uri_server with + | None -> assert false (* checked by vmx_source_of_arg *) + | Some server -> server in + let port = + match uri.uri_port with + | i when i <= 0 -> None + | i -> Some (string_of_int i) in + let user = uri.Xml.uri_user in + (* RHBZ#1774386 *) - if not (Ssh.remote_file_exists uri flat_vmdk) then + if not (Ssh.remote_file_exists ~password ?port ~server ?user + flat_vmdk) then error (f_"This transport does not support guests with snapshots. \ Either collapse the snapshots for this guest and try \ the conversion again, or use one of the alternate \ conversion methods described in \ virt-v2v-input-vmware(1) section \"NOTES\".");
- let server = Ssh.server_of_uri uri in - let port = Option.map string_of_int (Ssh.port_of_uri uri) in - let user = uri.Xml.uri_user in let cor = dir // "convert" in let bandwidth = options.bandwidth in let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password
OK (complicated, with *_of_uri being flattened in one go, but seems OK)
diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml index 94ae9957e3..99c86b1ae0 100644 --- a/input/parse_domain_from_vmx.ml +++ b/input/parse_domain_from_vmx.ml @@ -335,8 +335,21 @@ let parse_domain_from_vmx vmx_source = match vmx_source with | File filename -> Parse_vmx.parse_file filename | SSH (password, uri) ->
(3) So this is the pattern where -- corresponding to my other suggestion -- we'd have to bind "password" for real, because now we are passing it to "download_file".
+ let server = + match uri.uri_server with + | None -> assert false (* checked by vmx_source_of_arg *) + | Some server -> server in + let port = + match uri.uri_port with + | i when i <= 0 -> None + | i -> Some (string_of_int i) in + let user = uri.Xml.uri_user in + let path = + match uri.uri_path with + | None -> assert false (* checked by vmx_source_of_arg *) + | Some path -> path in let filename = tmpdir // "source.vmx" in - Ssh.download_file uri filename; + Ssh.download_file ~password ?port ~server ?user path filename; Parse_vmx.parse_file filename in
let name =
OK (although not a fan of the code duplication)
@@ -346,7 +359,10 @@ let parse_domain_from_vmx vmx_source = warning (f_"no displayName key found in VMX file"); match vmx_source with | File filename -> name_from_disk filename - | SSH (_, uri) -> name_from_disk (Ssh.path_of_uri uri) in + | SSH (_, uri) -> + match uri.uri_path with + | None -> assert false (* checked by vmx_source_of_arg *) + | Some path -> name_from_disk path in
let genid = (* See: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html *)
Hm. I consider this a bit of unwelcome fallout, especially with the same match on "uri.uri_path" as above (I do realize we evaluate different expressions on both "Some path" branches -- "path" versus "name_from_disk path"). (4) Can we introduce a helper function that takes a URI and returns a tuple (server, port, user, path)? Then it could be called from both "input_vmx.ml" and "parse_domain_from_vmx.ml", plus in the latter, we could use the path component of the returned tuple for both the "Ssh.download_file" call and for the "name_from_disk" call. ... In effect I'm suggesting to *replace* the *_of_uri helpers in the Ssh module with a new (unified) helper function in -- say -- Utils. If you agree with that idea, then it might be best to first perform that conversion / extraction in a separate patch, prepended to this one. Laszlo