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
_______________________________________________
Libguestfs mailing list -- guestfs@lists.libguestfs.org
To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
