On 1/17/24 14:24, Richard W.M. Jones wrote: > 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.
Very cool! I find OCaml relatively non-intuitive when it comes to taking parameters (well, because there are only single-arg functions in ocaml, I should say "when it comes to taking the parameter"), but I can't deny the syntax does seem to cover everything imaginable. > >>> 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. Thanks! > >>> + 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. OK. Works for me. Reviewed-by: Laszlo Ersek <ler...@redhat.com> _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
