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

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 "_".

> 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.