[Libguestfs] Re: [PATCH virt-v2v 4/6] virt-v2v: -i vmx: Remove dependency of ssh.ml on Xml.uri

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.