On Wed, Jan 17, 2024 at 05:21:53AM +0100, Laszlo Ersek wrote:

On 1/15/24 19:24, Richard W.M. Jones wrote:

...

This is a straight refactor of the existing code that handles ssh/scp into a new module. In this commit I just copy the code around without doing any cleanup; cleanup will follow in subsequent commits. --- input/Makefile.am | 6 ++- input/parse_domain_from_vmx.mli | 6 --- input/ssh.mli | 34 ++++++++++++++ input/input_vmx.ml | 8 ++-- input/parse_domain_from_vmx.ml | 60 ++----------------------- input/ssh.ml | 78 +++++++++++++++++++++++++++++++++ 6 files changed, 123 insertions(+), 69 deletions(-)

"git show -W --no-renames --color-moved=zebra --color" works very well on this patch.

Learning something new!

One comment below: [...]

...

diff --git a/input/ssh.ml b/input/ssh.ml new file mode 100644 index 0000000000..8e12899a8e --- /dev/null +++ b/input/ssh.ml @@ -0,0 +1,78 @@ +(* virt-v2v + * Copyright (C) 2009-2020 Red Hat Inc.

This is a new file; are you sure you don't want to update the (C) notice's end year to 2024?

Yes I do. I was going to also either update all the (C) dates or remove them, as you did in nbdkit: https://gitlab.com/nbdkit/nbdkit/-/commit/712fe24c18df21567e566a013d92f91386...

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks! Rich.

Laszlo

...

+ * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + *) + +open Std_utils +open Tools_utils +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 temporary local file, returning the path + * of the temporary local file. + *) +let scp_from_remote_to_temporary uri tmpdir filename = + let localfile = tmpdir // filename in + + let cmd = + sprintf "scp%s%s %s%s:%s %s" + (if verbose () then "" else " -q") + (match port_of_uri uri with + | None -> "" + | Some port -> sprintf " -P %d" port) + (match uri.Xml.uri_user with + | None -> "" + | Some user -> quote user ^ "@") + (quote (server_of_uri uri)) + (quote (path_of_uri uri)) + (quote localfile) in + if verbose () then + eprintf "%s\n%!" cmd; + if Sys.command cmd <> 0 then + error (f_"could not copy the VMX file from the remote server, \ + see earlier error messages"); + localfile + +(* Test if [path] exists on the remote server. *) +let remote_file_exists uri path = + let cmd = + sprintf "ssh%s %s%s test -f %s" + (match port_of_uri uri with + | None -> "" + | Some port -> sprintf " -p %d" port) + (match uri.Xml.uri_user with + | None -> "" + | Some user -> quote user ^ "@") + (quote (server_of_uri uri)) + (* 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 + *) + (quote (quote path)) in + if verbose () then + eprintf "%s\n%!" cmd; + Sys.command cmd = 0

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On 1/15/24 19:24, Richard W.M. Jones wrote:

The existing 'scp_from_remote_to_temporary' wrapper around scp was pretty weird (I think from much patching without refactoring). Simplify it so it no longer generates the output filename, and rename it accordingly to 'download_file' (as we will soon remove the need for and dependency on 'scp'). --- input/ssh.mli | 7 +++---- input/parse_domain_from_vmx.ml | 4 ++-- input/ssh.ml | 13 ++++--------- 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/input/ssh.mli b/input/ssh.mli index e9a1a6a8c7..62e78bd3e8 100644 --- a/input/ssh.mli +++ b/input/ssh.mli @@ -27,8 +27,7 @@ val port_of_uri : Xml.uri -> int option is ignored). *) val remote_file_exists : Xml.uri -> string -> bool

-(** [scp_from_remote_to_temporary ssh_uri tmpdir filename] +(** [download_file ssh_uri output] uses scp to copy the single remote file at [ssh_uri] to - the local file called [tmpdir/filename]. It returns the - final path [tmpdir/filename]. *) -val scp_from_remote_to_temporary : Xml.uri -> string -> string -> string + the local file called [output]. *) +val download_file : Xml.uri -> string -> unit diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml index 8cf5893c35..e6500da64a 100644 --- a/input/parse_domain_from_vmx.ml +++ b/input/parse_domain_from_vmx.ml @@ -335,8 +335,8 @@ let parse_domain_from_vmx vmx_source = match vmx_source with | File filename -> Parse_vmx.parse_file filename | SSH uri -> - let filename = Ssh.scp_from_remote_to_temporary uri tmpdir - "source.vmx" in + let filename = tmpdir // "source.vmx" in + Ssh.download_file uri filename; Parse_vmx.parse_file filename in

let name = diff --git a/input/ssh.ml b/input/ssh.ml index 8e12899a8e..fbc0e54f5d 100644 --- a/input/ssh.ml +++ b/input/ssh.ml @@ -32,12 +32,8 @@ let server_of_uri { Xml.uri_server } = let path_of_uri { Xml.uri_path } = match uri_path with None -> assert false | Some p -> p

-(* 'scp' a remote file into a temporary local file, returning the path - * of the temporary local file. - *) -let scp_from_remote_to_temporary uri tmpdir filename = - let localfile = tmpdir // filename in - +(* 'scp' a remote file into a local file. *) +let download_file uri output = let cmd = sprintf "scp%s%s %s%s:%s %s" (if verbose () then "" else " -q") @@ -49,13 +45,12 @@ let scp_from_remote_to_temporary uri tmpdir filename = | Some user -> quote user ^ "@") (quote (server_of_uri uri)) (quote (path_of_uri uri)) - (quote localfile) in + (quote output) in if verbose () then eprintf "%s\n%!" cmd; if Sys.command cmd <> 0 then error (f_"could not copy the VMX file from the remote server, \ - see earlier error messages"); - localfile + see earlier error messages")

(* Test if [path] exists on the remote server. *) let remote_file_exists uri path =

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On 1/15/24 19:24, Richard W.M. Jones wrote:

Since we use the input password in various places in the VMX module, store the input password in vmx_source. This neutral refactoring makes later changes simpler. --- input/parse_domain_from_vmx.mli | 7 ++++--- input/input_vmx.ml | 13 ++++++------- input/parse_domain_from_vmx.ml | 12 ++++++------ 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/input/parse_domain_from_vmx.mli b/input/parse_domain_from_vmx.mli index 42f8100ee7..208797a736 100644 --- a/input/parse_domain_from_vmx.mli +++ b/input/parse_domain_from_vmx.mli @@ -17,8 +17,9 @@ *)

type vmx_source = - | File of string (** local file or NFS *) - | SSH of Xml.uri (** SSH URI *) + | File of string (** local file or NFS *) + | SSH of Nbdkit_ssh.password * Xml.uri (** SSH URI *)

-val vmx_source_of_arg : [`SSH] option -> string -> vmx_source

[`SSH] looks unusual. According to <https://dev.realworldocaml.org/variants.html>, it is an "exact polymorphic variant type". If I understand correctly, originally we just wanted an empty data constructor (taking no params) to plug into "option", but without defining an explicit variant type. An alternative to "[`SSH] option" could have been: - first step: just define a normal variant type, with a single data constructor taking no parameter. This would effectively amount to an enumeration type, with a single (and flat) enumeration constant: type ssh_type = Ssh val vmx_source_of_arg : ssh_type option -> string -> vmx_source - second step: realizing that the value set {None, Some Ssh} is not super useful, we could have gone with type ssh_type = Ssh | Nothing val vmx_source_of_arg : ssh_type -> string -> vmx_source effectively squashing "option" into "ssh_type" (the same two-element value set would be represented directly in the enumeration type). Is that right? (This is not a comment on the patch, just me trying to understand the pre-patch code.)

+val vmx_source_of_arg : Nbdkit_ssh.password -> [`SSH] option -> string -> + vmx_source val parse_domain_from_vmx : vmx_source -> Types.source * string list diff --git a/input/input_vmx.ml b/input/input_vmx.ml index bd20420ca0..b9cce10fed 100644 --- a/input/input_vmx.ml +++ b/input/input_vmx.ml @@ -45,13 +45,17 @@ module VMX = struct let vmx_source = match args with | [arg] -> + let input_password = + match options.input_password with + | None -> Nbdkit_ssh.NoPassword + | Some ip -> Nbdkit_ssh.PasswordFile ip in

Huh, seems like a very comfortable fit!

let input_transport = match options.input_transport with | None -> None | Some `SSH -> Some `SSH | Some `VDDK -> error (f_"-i vmx: cannot use -it vddk in this input mode") in - vmx_source_of_arg input_transport arg + vmx_source_of_arg input_password input_transport arg | _ -> error (f_"-i vmx: expecting a VMX file or ssh:// URI") in

@@ -73,7 +77,7 @@ module VMX = struct On_exit.kill pid ) filenames

- | SSH uri -> + | SSH (password, uri) -> List.iteri ( fun i filename -> let socket = sprintf "%s/in%d" dir i in @@ -95,11 +99,6 @@ module VMX = struct 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 password = - match options.input_password with - | None -> Nbdkit_ssh.NoPassword - | Some ip -> Nbdkit_ssh.PasswordFile ip in - let cor = dir // "convert" in let bandwidth = options.bandwidth in let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password

Ah, so this "comfortable fit" is already there, we're just moving the computation. OK.

diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml index e6500da64a..94ae9957e3 100644 --- a/input/parse_domain_from_vmx.ml +++ b/input/parse_domain_from_vmx.ml @@ -29,13 +29,13 @@ open Utils open Name_from_disk

type vmx_source = - | File of string (* local file or NFS *) - | SSH of Xml.uri (* SSH URI *) + | File of string (* local file or NFS *) + | SSH of Nbdkit_ssh.password * Xml.uri (* SSH URI *)

(* The single filename on the command line is intepreted either as * a local file or a remote SSH URI (only if ‘-it ssh’). *) -let vmx_source_of_arg input_transport arg = +let vmx_source_of_arg input_password input_transport arg = match input_transport, arg with | None, arg -> File arg | Some `SSH, arg -> @@ -49,7 +49,7 @@ let vmx_source_of_arg input_transport arg = error (f_"vmx URI remote server name omitted"); if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then error (f_"vmx URI path component looks incorrect"); - SSH uri + SSH (input_password, uri)

let rec find_disks vmx vmx_source = (* Set the s_disk_id field to an incrementing number. *) @@ -334,7 +334,7 @@ let parse_domain_from_vmx vmx_source = let vmx = match vmx_source with | File filename -> Parse_vmx.parse_file filename - | SSH uri -> + | SSH (password, uri) -> let filename = tmpdir // "source.vmx" in Ssh.download_file uri filename; Parse_vmx.parse_file filename in

(1) Seems like binding the first component of the tuple with "password" is unnecessary here -- I think we could use the wildcard "_", just like below:

@@ -346,7 +346,7 @@ 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) -> name_from_disk (Ssh.path_of_uri uri) in

let genid = (* See: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html *)

Now, I figure that in one of the next patches, the part marked with (1) will actually grow a use for the password component of the tuple; after all, "Ssh.download_file" will still need to authenticate. ... Right, that seems to be happening in the next patch (#4). For slightly improving clarity, do you think you could use the wildcard "_" at (1) in *this* patch, and move the *real* binding to the next patch? Because right in this patch, the binding is still unneeded. (2) A further question: now I wanted to recheck how "Ssh.download_file uri" actually uses the password file passed in with "-ip", and the answer, as far as I can tell, is that it doesn't! The manual <https://libguestfs.org/virt-v2v-input-vmware.1.html> says, Note that support for non-interactive authentication via the -ip option is incomplete. Some operations remain that still require the user to enter the password manually. Therefore ssh-agent is recommended over the -ip option. See https://bugzilla.redhat.com/1854275. which certainly brings back some unpleasant :) memories -- and the BZ is indeed called 'document that vmx+ssh "-ip" auth doesn't cover ssh / scp shell commands'. OK, so long story short: is it the case that this patch series actually *extends* "-ip" support to "-i vmx -it ssh"? Because in that case, I think we should update the documentation as well. (3) Having written the above, I realize that in patch #4, we don't actually *consume* the password (file) just yet; we only push it down one level deeper. The actual consumption only happens in patch#6, where we finally forward ~password to "start_nbdkit". Because of that, I'm going to suggest one more "tightening" here (not sure if it makes sense; my OCaml is certainly rusty): in patch#4, can we *implement* (not declare) the "download_file" function such that it still take (bind) the password with the _ wildcard? Quick check: # let func _ a b = a + b;; val func : 'a -> int -> int -> int = <fun> # func "hello" 1 2;; - : int = 3 so at least the syntax should exist for this. And then bind the password in the "download_file" function for real only in patch#6, where it gets put to use -- i.e., where it gets passed to "start_nbdkit". "Unused variables / parameters" don't hurt, but at least in OCaml, we can keep them nameless! :) Summary: - I think at (1) we can / should use "_" - I suggest adding a patch#7 that removes the reference to RHBZ 1854275 from the docs, because (I think?) we're ending up fixing that BZ now - I'll make comment (3) under patch#4 too. Only the first item affects this particular patch, and I don't insist on even that; just consider it. However you decide: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo

On Wed, Jan 17, 2024 at 06:23:33AM +0100, Laszlo Ersek wrote:

On 1/15/24 19:24, Richard W.M. Jones wrote:

...

Since we use the input password in various places in the VMX module, store the input password in vmx_source. This neutral refactoring makes later changes simpler. --- input/parse_domain_from_vmx.mli | 7 ++++--- input/input_vmx.ml | 13 ++++++------- input/parse_domain_from_vmx.ml | 12 ++++++------ 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/input/parse_domain_from_vmx.mli b/input/parse_domain_from_vmx.mli index 42f8100ee7..208797a736 100644 --- a/input/parse_domain_from_vmx.mli +++ b/input/parse_domain_from_vmx.mli @@ -17,8 +17,9 @@ *)

type vmx_source = - | File of string (** local file or NFS *) - | SSH of Xml.uri (** SSH URI *) + | File of string (** local file or NFS *) + | SSH of Nbdkit_ssh.password * Xml.uri (** SSH URI *)

-val vmx_source_of_arg : [`SSH] option -> string -> vmx_source

[`SSH] looks unusual. According to <https://dev.realworldocaml.org/variants.html>, it is an "exact polymorphic variant type".

If I understand correctly, originally we just wanted an empty data constructor (taking no params) to plug into "option", but without defining an explicit variant type.

An alternative to "[`SSH] option" could have been:

- first step: just define a normal variant type, with a single data constructor taking no parameter. This would effectively amount to an enumeration type, with a single (and flat) enumeration constant:

type ssh_type = Ssh val vmx_source_of_arg : ssh_type option -> string -> vmx_source

- second step: realizing that the value set {None, Some Ssh} is not super useful, we could have gone with

type ssh_type = Ssh | Nothing val vmx_source_of_arg : ssh_type -> string -> vmx_source

effectively squashing "option" into "ssh_type" (the same two-element value set would be represented directly in the enumeration type).

Is that right?

No I don't think that's right. The actual type of options.input_transport is: input_transport : [`SSH|`VDDK] option; (defined in input/input.mli). What we're saying here is that you can only call vmx_source_of_arg with specifically the subset of this type which is Some `SSH (or None). Some `SSH represents SSH connections (-it ssh), and None (no -it option) represents local files. The caller of vmx_source_of_arg ensures this using: let input_transport = match options.input_transport with | None -> None | Some `SSH -> Some `SSH | Some `VDDK -> error (f_"-i vmx: cannot use -it vddk in this input mode") in vmx_source_of_arg input_password input_transport arg (input/input_vmx.ml) and the type checker verifies this is correct at compile time.

(This is not a comment on the patch, just me trying to understand the pre-patch code.)

...

+val vmx_source_of_arg : Nbdkit_ssh.password -> [`SSH] option -> string -> + vmx_source val parse_domain_from_vmx : vmx_source -> Types.source * string list diff --git a/input/input_vmx.ml b/input/input_vmx.ml index bd20420ca0..b9cce10fed 100644 --- a/input/input_vmx.ml +++ b/input/input_vmx.ml @@ -45,13 +45,17 @@ module VMX = struct let vmx_source = match args with | [arg] -> + let input_password = + match options.input_password with + | None -> Nbdkit_ssh.NoPassword + | Some ip -> Nbdkit_ssh.PasswordFile ip in

Huh, seems like a very comfortable fit!

...

let input_transport = match options.input_transport with | None -> None | Some `SSH -> Some `SSH | Some `VDDK -> error (f_"-i vmx: cannot use -it vddk in this input mode") in - vmx_source_of_arg input_transport arg + vmx_source_of_arg input_password input_transport arg | _ -> error (f_"-i vmx: expecting a VMX file or ssh:// URI") in

@@ -73,7 +77,7 @@ module VMX = struct On_exit.kill pid ) filenames

- | SSH uri -> + | SSH (password, uri) -> List.iteri ( fun i filename -> let socket = sprintf "%s/in%d" dir i in @@ -95,11 +99,6 @@ module VMX = struct 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 password = - match options.input_password with - | None -> Nbdkit_ssh.NoPassword - | Some ip -> Nbdkit_ssh.PasswordFile ip in - let cor = dir // "convert" in let bandwidth = options.bandwidth in let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password

Ah, so this "comfortable fit" is already there, we're just moving the computation. OK.

...

diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml index e6500da64a..94ae9957e3 100644 --- a/input/parse_domain_from_vmx.ml +++ b/input/parse_domain_from_vmx.ml @@ -29,13 +29,13 @@ open Utils open Name_from_disk

type vmx_source = - | File of string (* local file or NFS *) - | SSH of Xml.uri (* SSH URI *) + | File of string (* local file or NFS *) + | SSH of Nbdkit_ssh.password * Xml.uri (* SSH URI *)

(* The single filename on the command line is intepreted either as * a local file or a remote SSH URI (only if ‘-it ssh’). *) -let vmx_source_of_arg input_transport arg = +let vmx_source_of_arg input_password input_transport arg = match input_transport, arg with | None, arg -> File arg | Some `SSH, arg -> @@ -49,7 +49,7 @@ let vmx_source_of_arg input_transport arg = error (f_"vmx URI remote server name omitted"); if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then error (f_"vmx URI path component looks incorrect"); - SSH uri + SSH (input_password, uri)

let rec find_disks vmx vmx_source = (* Set the s_disk_id field to an incrementing number. *) @@ -334,7 +334,7 @@ let parse_domain_from_vmx vmx_source = let vmx = match vmx_source with | File filename -> Parse_vmx.parse_file filename - | SSH uri -> + | SSH (password, uri) -> let filename = tmpdir // "source.vmx" in Ssh.download_file uri filename; Parse_vmx.parse_file filename in

(1) Seems like binding the first component of the tuple with "password" is unnecessary here -- I think we could use the wildcard "_", just like below:

...

@@ -346,7 +346,7 @@ 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) -> name_from_disk (Ssh.path_of_uri uri) in

let genid = (* See: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html *)

Now, I figure that in one of the next patches, the part marked with (1) will actually grow a use for the password component of the tuple; after all, "Ssh.download_file" will still need to authenticate.

... Right, that seems to be happening in the next patch (#4).

For slightly improving clarity, do you think you could use the wildcard "_" at (1) in *this* patch, and move the *real* binding to the next patch? Because right in this patch, the binding is still unneeded.

OK.

(2) A further question: now I wanted to recheck how "Ssh.download_file uri" actually uses the password file passed in with "-ip", and the answer, as far as I can tell, is that it doesn't!

The manual <https://libguestfs.org/virt-v2v-input-vmware.1.html> says,

Note that support for non-interactive authentication via the -ip option is incomplete. Some operations remain that still require the user to enter the password manually. Therefore ssh-agent is recommended over the -ip option. See https://bugzilla.redhat.com/1854275.

which certainly brings back some unpleasant :) memories -- and the BZ is indeed called 'document that vmx+ssh "-ip" auth doesn't cover ssh / scp shell commands'.

OK, so long story short: is it the case that this patch series actually *extends* "-ip" support to "-i vmx -it ssh"? Because in that case, I think we should update the documentation as well.

This does fix the -ip option (in a later patch). However I'm not sure if it fixes it everywhere. I guess so ... but I'm not too confident! Maybe we should leave the advice there?

(3) Having written the above, I realize that in patch #4, we don't actually *consume* the password (file) just yet; we only push it down one level deeper. The actual consumption only happens in patch#6, where we finally forward ~password to "start_nbdkit".

Because of that, I'm going to suggest one more "tightening" here (not sure if it makes sense; my OCaml is certainly rusty): in patch#4, can we *implement* (not declare) the "download_file" function such that it still take (bind) the password with the _ wildcard?

Sure.

Quick check:

# let func _ a b = a + b;; val func : 'a -> int -> int -> int = <fun>

# func "hello" 1 2;; - : int = 3

so at least the syntax should exist for this.

And then bind the password in the "download_file" function for real only in patch#6, where it gets put to use -- i.e., where it gets passed to "start_nbdkit".

"Unused variables / parameters" don't hurt, but at least in OCaml, we can keep them nameless! :)

Summary:

- I think at (1) we can / should use "_"

- I suggest adding a patch#7 that removes the reference to RHBZ 1854275 from the docs, because (I think?) we're ending up fixing that BZ now

- I'll make comment (3) under patch#4 too.

Only the first item affects this particular patch, and I don't insist on even that; just consider it. However you decide:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

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

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.

...

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.

...

+ 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. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On 1/15/24 19:24, Richard W.M. Jones wrote:

This filter retries a request on failure. Add a flag to make this filter optional, but default to using it if available (so there's no change to default behaviour).

The reason we want to make this option is that the next commit will want to use nbdkit-ssh-plugin to probe to see if a remote file is present, and we don't want to retry that probe on failure. --- input/nbdkit_ssh.mli | 1 + input/nbdkit_ssh.ml | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/input/nbdkit_ssh.mli b/input/nbdkit_ssh.mli index bc798c5a73..daa20bb2a5 100644 --- a/input/nbdkit_ssh.mli +++ b/input/nbdkit_ssh.mli @@ -25,6 +25,7 @@ type password =

val create_ssh : ?bandwidth:Types.bandwidth -> ?cor:string -> + ?retry:bool -> password:password -> ?port:string -> server:string -> diff --git a/input/nbdkit_ssh.ml b/input/nbdkit_ssh.ml index 3f6da9b62d..3b72d3adb3 100644 --- a/input/nbdkit_ssh.ml +++ b/input/nbdkit_ssh.ml @@ -44,7 +44,8 @@ let error_unless_nbdkit_min_version config = error_unless_nbdkit_version_ge config nbdkit_min_version

(* Create an nbdkit module specialized for reading from SSH sources. *) -let create_ssh ?bandwidth ?cor ~password ?port ~server ?user path = +let create_ssh ?bandwidth ?cor ?(retry=true) + ~password ?port ~server ?user path = if not (Nbdkit.is_installed ()) then error (f_"nbdkit is not installed or not working");

@@ -67,7 +68,8 @@ let create_ssh ?bandwidth ?cor ~password ?port ~server ?user path = (* Retry filter (if it exists) can be used to get around brief * interruptions in service. It must be closest to the plugin. *) - Nbdkit.add_filter_if_available cmd "retry"; + if retry then + Nbdkit.add_filter_if_available cmd "retry";

(* Caching extents speeds up qemu-img, especially its consecutive * block_status requests with req_one=1.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On Mon, Jan 15, 2024 at 06:24:16PM +0000, Richard W.M. Jones wrote:

If you use a -i vmx ssh filename containing '*' then it will expand the glob at the remote side. New scp is weird and silently creates a directory on the local side, eg:

scp 'remote:/tmp/test*' '/tmp/test*'

if there are multiple files matching the wildcard /tmp/test*, will create a local directory literally called "/tmp/test*/" and put the files into it. Who expected any of that?

This explanation above wasn't great. In my revised version of the commit I changed this to: If you use a -i vmx ssh filename containing '*' then it will expand the glob at the remote side. New scp is weird and silently creates a directory on the local side. For example suppose there's a remote file literally called "/tmp/test*" (ie. the '*' is part of the filename) and you thought you could copy that to local using: scp 'remote:/tmp/test*' '/tmp/test*' scp treats the first parameter (only) as a wildcard and if there are multiple files matching the wildcard /tmp/test*, will create a local directory literally called "/tmp/test*/" and put the files into it. Who expected any of that? Rich.

You might think that double quoting (as we used to use) might work, but that breaks with spaces and quotes. I guess scp is using different paths internally for glob versus everything else.

The only way to really make this work is to stop using scp entirely and just use sftp directly. The easiest way to use sftp is to use nbdkit-ssh-plugin. We already depend on nbdkit-ssh-plugin, nbdcopy and nbdinfo for other parts of virt-v2v, so might as well use the whole lot here.

One advantage of this change is that now the -ip (input password) parameter actually works in -ivmx -it ssh mode.

Other approaches that would have been possible:

- Use the OCaml NBD library instead of nbdcopy/nbdinfo

- Use 'nbdkit -U - ssh --run ...'

See also commit e2af12ba69c4463bb73d30db63290a887cdd41eb ("input: -i vmx: Remove support for openssh scp < 8.8")

See also commit 22c5b98ab78c734b478c26e14ee62e2a065aaa0c ("-it ssh: Double quote ssh command which tests remote file exists")

See also https://unix.stackexchange.com/a/587710

Reported-by: Ming Xie Fixes: https://issues.redhat.com/browse/RHEL-21365 --- input/ssh.mli | 7 ++++-- input/ssh.ml | 66 +++++++++++++++++++++++++-------------------------- 2 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/input/ssh.mli b/input/ssh.mli index 1f39cd1bea..40843024de 100644 --- a/input/ssh.mli +++ b/input/ssh.mli @@ -16,7 +16,10 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. *)

-(** Wrappers for finding and downloading remote files over ssh. *) +(** Wrappers for finding and downloading remote files over ssh. + + Internally this uses nbdkit-ssh-plugin (which uses sftp) as + that is much more predictable than running external ssh / scp. *)

(** [remote_file_exists password ?port server ?user path] checks that [path] exists on the remote server. *) @@ -25,7 +28,7 @@ val remote_file_exists : password:Nbdkit_ssh.password -> string -> bool

(** [download_file password ?port server ?user path output] - uses scp to copy the single remote file at [path] to + downloads the single remote file at [path] to the local file called [output]. *) val download_file : password:Nbdkit_ssh.password -> ?port:string -> server:string -> ?user:string -> string -> diff --git a/input/ssh.ml b/input/ssh.ml index 4e0e420e20..dd435ddc37 100644 --- a/input/ssh.ml +++ b/input/ssh.ml @@ -18,46 +18,46 @@

open Std_utils open Tools_utils +open Unix_utils open Common_gettext.Gettext

open Printf

-(* 'scp' a remote file into a local file. *) +let start_nbdkit ~password ?port ~server ?user path = + (* Create a random location for the socket used to talk to nbdkit. *) + let sockdir = Mkdtemp.temp_dir "v2vssh." in + On_exit.rm_rf sockdir; + let id = unique () in + let socket = sockdir // sprintf "nbdkit%d.sock" id in + + (* Note: Disabling the retry filter helps in the missing file case, + * otherwise nbdkit takes ages to time out. We're not expecting that + * the VMX file is large, so using this filter isn't necessary. + *) + let nbdkit = + Nbdkit_ssh.create_ssh ~retry:false ~password ~server ?port ?user path in + Nbdkit.set_readonly nbdkit true; + let _, pid = Nbdkit.run_unix socket nbdkit in + On_exit.kill pid; + + (* Return the URI of nbdkit. *) + "nbd+unix://?socket=" ^ socket + +(* Download a remote file into a local file. *) let download_file ~password ?port ~server ?user path output = - let cmd = - sprintf "scp%s%s %s%s:%s %s" - (if verbose () then "" else " -q") - (match port with - | None -> "" - | Some port -> sprintf " -P %s" port) - (match user with - | None -> "" - | Some user -> quote user ^ "@") - (quote server) - (quote path) - (quote output) in - if verbose () then - eprintf "%s\n%!" cmd; - if Sys.command cmd <> 0 then + let uri = start_nbdkit ~password ?port ~server ?user path in + + let cmd = [ "nbdcopy"; uri; output ] in + if run_command cmd <> 0 then error (f_"could not copy the VMX file from the remote server, \ see earlier error messages")

(* Test if [path] exists on the remote server. *) let remote_file_exists ~password ?port ~server ?user path = - let cmd = - sprintf "ssh%s %s%s test -f %s" - (match port with - | None -> "" - | Some port -> sprintf " -p %s" port) - (match user with - | None -> "" - | Some user -> quote user ^ "@") - (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 - *) - (quote (quote path)) in - if verbose () then - eprintf "%s\n%!" cmd; - Sys.command cmd = 0 + let uri = start_nbdkit ~password ?port ~server ?user path in + + (* Testing for remote size using nbdinfo should be sufficient to + * prove the remote file exists. + *) + let cmd = [ "nbdinfo"; "--size"; uri ] in + run_command cmd = 0 -- 2.43.0 _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-leave@lists.libguestfs.org

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On Wed, Jan 17, 2024 at 09:24:59AM +0100, Laszlo Ersek wrote:

On 1/15/24 19:24, Richard W.M. Jones wrote:

...

If you use a -i vmx ssh filename containing '*' then it will expand the glob at the remote side. New scp is weird and silently creates a directory on the local side, eg:

scp 'remote:/tmp/test*' '/tmp/test*'

if there are multiple files matching the wildcard /tmp/test*, will create a local directory literally called "/tmp/test*/" and put the files into it. Who expected any of that?

You might think that double quoting (as we used to use) might work, but that breaks with spaces and quotes. I guess scp is using different paths internally for glob versus everything else.

The only way to really make this work is to stop using scp entirely and just use sftp directly. The easiest way to use sftp is to use nbdkit-ssh-plugin. We already depend on nbdkit-ssh-plugin, nbdcopy and nbdinfo for other parts of virt-v2v, so might as well use the whole lot here.

One advantage of this change is that now the -ip (input password) parameter actually works in -ivmx -it ssh mode.

... single-handedly resolving RHBZ 1854275, at long last :)

((1) we should update <https://libguestfs.org/virt-v2v-input-vmware.1.html>, likely in a separate patch.)

I'm going to inspect the code to see if there are other possible places where we might be running ssh / scp directly first. It'll be in another patch.

...

Other approaches that would have been possible:

- Use the OCaml NBD library instead of nbdcopy/nbdinfo

- Use 'nbdkit -U - ssh --run ...'

See also commit e2af12ba69c4463bb73d30db63290a887cdd41eb ("input: -i vmx: Remove support for openssh scp < 8.8")

See also commit 22c5b98ab78c734b478c26e14ee62e2a065aaa0c ("-it ssh: Double quote ssh command which tests remote file exists")

See also https://unix.stackexchange.com/a/587710

Reported-by: Ming Xie Fixes: https://issues.redhat.com/browse/RHEL-21365 --- input/ssh.mli | 7 ++++-- input/ssh.ml | 66 +++++++++++++++++++++++++-------------------------- 2 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/input/ssh.mli b/input/ssh.mli index 1f39cd1bea..40843024de 100644 --- a/input/ssh.mli +++ b/input/ssh.mli @@ -16,7 +16,10 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. *)

-(** Wrappers for finding and downloading remote files over ssh. *) +(** Wrappers for finding and downloading remote files over ssh. + + Internally this uses nbdkit-ssh-plugin (which uses sftp) as + that is much more predictable than running external ssh / scp. *)

(** [remote_file_exists password ?port server ?user path] checks that [path] exists on the remote server. *) @@ -25,7 +28,7 @@ val remote_file_exists : password:Nbdkit_ssh.password -> string -> bool

(** [download_file password ?port server ?user path output] - uses scp to copy the single remote file at [path] to + downloads the single remote file at [path] to the local file called [output]. *) val download_file : password:Nbdkit_ssh.password -> ?port:string -> server:string -> ?user:string -> string -> diff --git a/input/ssh.ml b/input/ssh.ml index 4e0e420e20..dd435ddc37 100644 --- a/input/ssh.ml +++ b/input/ssh.ml @@ -18,46 +18,46 @@

open Std_utils open Tools_utils +open Unix_utils open Common_gettext.Gettext

open Printf

-(* 'scp' a remote file into a local file. *) +let start_nbdkit ~password ?port ~server ?user path = + (* Create a random location for the socket used to talk to nbdkit. *) + let sockdir = Mkdtemp.temp_dir "v2vssh." in + On_exit.rm_rf sockdir; + let id = unique () in + let socket = sockdir // sprintf "nbdkit%d.sock" id in

(2) Vague memory: do we need to do anything about the file mode bits / owner / group of the directory and/or socket?

So my guess here is no, because ... Mkdtemp.temp_dir is a wrapper around the POSIX mkdtemp function, which creates the directory with 0700 (ignoring umask). The socket is opened by our own subprocess, nbdinfo or nbdcopy. This is different from the previous case where we ran qemu from libvirt and libvirt (when running as root) runs qemu as qemu:qemu, and so we had to take special steps with the socket. AFAIK there's no case where a subprocess wouldn't be able to immediately open the socket. (Well there is: SELinux. But that would be a policy error.)

...

+ + (* Note: Disabling the retry filter helps in the missing file case, + * otherwise nbdkit takes ages to time out. We're not expecting that + * the VMX file is large, so using this filter isn't necessary. + *) + let nbdkit = + Nbdkit_ssh.create_ssh ~retry:false ~password ~server ?port ?user path in + Nbdkit.set_readonly nbdkit true; + let _, pid = Nbdkit.run_unix socket nbdkit in + On_exit.kill pid; + + (* Return the URI of nbdkit. *) + "nbd+unix://?socket=" ^ socket + +(* Download a remote file into a local file. *) let download_file ~password ?port ~server ?user path output =

(3) This is where we'd bind the password with "password" rather than with "_", as this is the first time we put it to use.

Yup, updated.

...

- let cmd = - sprintf "scp%s%s %s%s:%s %s" - (if verbose () then "" else " -q") - (match port with - | None -> "" - | Some port -> sprintf " -P %s" port) - (match user with - | None -> "" - | Some user -> quote user ^ "@") - (quote server) - (quote path) - (quote output) in - if verbose () then - eprintf "%s\n%!" cmd; - if Sys.command cmd <> 0 then + let uri = start_nbdkit ~password ?port ~server ?user path in + + let cmd = [ "nbdcopy"; uri; output ] in + if run_command cmd <> 0 then error (f_"could not copy the VMX file from the remote server, \ see earlier error messages")

Nice!

...

(* Test if [path] exists on the remote server. *) let remote_file_exists ~password ?port ~server ?user path = - let cmd = - sprintf "ssh%s %s%s test -f %s" - (match port with - | None -> "" - | Some port -> sprintf " -p %s" port) - (match user with - | None -> "" - | Some user -> quote user ^ "@") - (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 - *) - (quote (quote path)) in - if verbose () then - eprintf "%s\n%!" cmd; - Sys.command cmd = 0 + let uri = start_nbdkit ~password ?port ~server ?user path in + + (* Testing for remote size using nbdinfo should be sufficient to + * prove the remote file exists. + *) + let cmd = [ "nbdinfo"; "--size"; uri ] in + run_command cmd = 0

Also nice.

I think the only relevant request of mine is to update the manual.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v