On 1/17/24 14:24, Richard W.M. Jones wrote:
On Wed, Jan 17, 2024 at 07:17:39AM +0100, Laszlo Ersek wrote:
On 1/15/24 19:24, Richard W.M. Jones wrote:
-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 "_".
Since the parameter is labelled you can't replace the parameter with '_', since that means the function type changes.
However (I discovered today) you can use ~password:_ which doesn't bind password inside the function:
# let f ~password a = a ;; val f : password:'a -> 'b -> 'b = <fun> # let f _ a = a ;; val f : 'a -> 'b -> 'b = <fun> # let f ~password a = password ;; val f : password:'a -> 'b -> 'a = <fun> # let f ~password:_ a = password ;; Error: Unbound value password
So I used ~password:_ in the updated patch.
Very cool! I find OCaml relatively non-intuitive when it comes to taking parameters (well, because there are only single-arg functions in ocaml, I should say "when it comes to taking the parameter"), but I can't deny the syntax does seem to cover everything imaginable.
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".
I fixed this in the updated patch, so now patch #3 uses SSH (_, uri) and patch #4 (this patch) replaces the _ with password.
Thanks!
+ 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.
I'm lukewarm on this one. I think just showing what the code does is easier to read than adding a rarely used library function elsewhere.
OK. Works for me. Reviewed-by: Laszlo Ersek <lersek@redhat.com>