On 1/17/24 14:10, Richard W.M. Jones wrote:
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.
Ah, that's novel. This is how the union is restricted to a subset, and the compiler can enforce it. Very nice. I think this is the first time I'm grasping (I hope...) polymorphic variants!
(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?
Ask Ming & team to check -ip exhaustively with the patches in place? I vaguely recall this was all that was missing, but I'm not 100% sure.
(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.
Thanks Laszlo