[Libguestfs] Re: [PATCH virt-v2v 3/6] virt-v2v: -i vmx: Add the input password to vmx_source

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(a)redhat.com&gt;