Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

virt-v2v 2.2.0 not handling vmware guests with spaces in the name #35

Open
bsanders opened this issue Sep 25, 2023 · 13 comments
Open

virt-v2v 2.2.0 not handling vmware guests with spaces in the name #35

bsanders opened this issue Sep 25, 2023 · 13 comments

Comments

@bsanders
Copy link

virt-v2v 2.2.0 (as packaged in debian 12) fails to convert:

/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test.vmx using -it ssh for VMware.

Specifically, the error in question:

virt-v2v -v -x -o local -of qcow2 -oa sparse -os /tmp/staging/esxi-host/converted/bsanders_win_test -on bsanders_win_test -i vmx -it ssh ssh://root@esxi-host/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders%20win%20test/bsanders%20win%20test.vmx
<snip of lots of debug output I can happily provide>
ssh 'root'@'esxi-host' test -f '/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test-flat.vmdk'
sh: win: unknown operand
virt-v2v: error: 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".
Unix.Unix_error(Unix.ENOENT, "unlink", "/tmp/v2v.dfNaCn/in0")
rm -rf -- '/tmp/vmx.lGuf8v'
rm -rf -- '/tmp/v2v.dfNaCn'

Note the test -f command. The documentation [0] suggested to convert characters to %20, and in virt-v2v 1.4, this works as expected. The error message is inaccurate - this VM does not have snapshots.

The same ssh test line from v2v 1.4, note the subtle difference in quoting:

ssh 'root'@'esxi-host' test -f ''\''/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test-flat.vmdk'\'''

[0] https://libguestfs.org/virt-v2v-input-vmware.1.html - "VMX: Construct the SSH URI"
@rwmjones
Copy link
Member

This is probably because of this change upstream: e2af12b

Unfortunately there's no syntax that works for both versions of scp :-(

@rwmjones
Copy link
Member

IIRC it's the client (virt-v2v) side scp which matters, not the server side, although I'm not 100% sure.

@rwmjones
Copy link
Member

And to confuse things, some distros backport the parsing changes from 8.8 but still call it "OpenSSH 8.7". RHEL 9 is one of these. Not sure about Debian but you might want to check.

@bsanders
Copy link
Author

Hmmm. The virt-v2v commit above is to support the "newer" sftp style, right? I think the Debian scp should be using that, as it's fairly new as well, and I don't see any patches that would be reverting it.

Debian 12 says its package is:

# apt info openssh-client
Package: openssh-client
Version: 1:9.2p1-2

Reading the changelog in the src package, I saw this

openssh (1:9.0p1-1) unstable; urgency=medium

  * New upstream release (https://www.openssh.com/releasenotes.html#9.0p1):
    - scp(1): Use the SFTP protocol by default (closes: #144579, #204546,
      #327019). This changes scp's quoting semantics by no longer performing
      wildcard expansion using the remote shell, and (with some server
      versions) no longer expanding ~user paths. The -O option is available
      to use the old protocol. See NEWS.Debian for more details.

<snip>
 -- Colin Watson <cjwatson@debian.org>  Sat, 09 Apr 2022 14:14:10 +0100

Interestingly, Ubuntu 22.04 (where I'm running virt-v2v 1.44.2 (not 1.4 as I typo'd above)) has

Version: 1:8.9p1-3ubuntu0.4

Glancing at the patches in the debian12 package [0], nothing stands out to me other than the very short scp-quoting.patch [1], which I think is actually unrelated (it appears to apply only to verbose mode)? The Ubuntu src package has the same patch, in any case. A grep for quot in that directory didn't turn up anything relevant either.

[0]

openssh-client-src-pkg/debian# ls -1 patches/
authorized-keys-man-symlink.patch
conch-ssh-rsa.patch
debian-banner.patch
debian-config.patch
dnssec-sshfp.patch
doc-hash-tab-completion.patch
gnome-ssh-askpass2-icon.patch
gssapi.patch
keepalive-extensions.patch
maxhostnamelen.patch
mention-ssh-keygen-on-keychange.patch
no-openssl-version-status.patch
openbsd-docs.patch
package-versioning.patch
remove-spurious-ssh-agent-options.patch
restore-authorized_keys2.patch
restore-tcp-wrappers.patch
revert-ipqos-defaults.patch
scp-quoting.patch
selinux-role.patch
series
shell-path.patch
ssh-agent-setgid.patch
ssh-argv0.patch
ssh-vulnkey-compat.patch
syslog-level-silent.patch
systemd-readiness.patch
systemd-socket-activation.patch
user-group-modes.patch

[1]

diff --git a/scp.c b/scp.c
index 1adff5cec..df590c4e3 100644
--- a/scp.c
+++ b/scp.c
@@ -239,8 +239,16 @@ do_local_cmd(arglist *a)
 
        if (verbose_mode) {
                fprintf(stderr, "Executing:");
-               for (i = 0; i < a->num; i++)
-                       fmprintf(stderr, " %s", a->list[i]);
+               for (i = 0; i < a->num; i++) {
+                       if (i == 0)
+                               fmprintf(stderr, " %s", a->list[i]);
+                       else
+                               /*
+                                * TODO: misbehaves if a->list[i] contains a
+                                * single quote
+                                */
+                               fmprintf(stderr, " '%s'", a->list[i]);
+               }
                fprintf(stderr, "\n");
        }
        if ((pid = fork()) == -1)

@rwmjones
Copy link
Member

So perhaps it's not that patch, as the patch applies to scp not ssh. The command being run is ssh.

What happens if you run the command by hand, ie:

ssh 'root'@'esxi-host' test -f '/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test-flat.vmdk'

There may be a second quoting issue here. I created a file called /var/tmp/foo bar on a remote machine (not ESXi) and then had to double quote to get commands to work, eg:

$ ssh foo test -f "/var/tmp/foo bar"
bash: line 1: test: /var/tmp/foo: binary operator expected
$ ssh foo stat "'/var/tmp/foo bar'"
  File: /var/tmp/foo bar
[etc]

@rwmjones
Copy link
Member

I believe the code affected is:

virt-v2v/input/parse_domain_from_vmx.ml

Line 90 in c5c9aa2

let remote_file_exists uri path =

@lersek @mxie91

@bsanders
Copy link
Author

I get the same output from these two commands on Debian12, Ubuntu 22.04, Fedora 39 (in Vagrant), and CentOS Stream 9 (also random vagrant box). The weird double-single-escaped quote works, but the plain single quote doesn't.

ssh 'root'@'esxi-host' test -f '/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test-flat.vmdk'
sh: win: unknown operand
ssh 'root'@'esxi-host' test -f ''\''/vmfs/volumes/630cd6f1-0adec4cd-8b43-e0d55e25f3e4/bsanders win test/bsanders win test-flat.vmdk'\'''

@lersek
Copy link
Collaborator

lersek commented Sep 30, 2023

My impression is we need to revert the last hunk of commit e2af12b, i.e., the hunk that removes the double-quoting from remote_file_exists.

@lersek
Copy link
Collaborator

lersek commented Sep 30, 2023 •
edited

(... Because that commit removed the double quoting for both scp and ssh. The update was probably right for (recent versions of) scp, but ssh continues needing the double quoting -- one layer for the local shell, another for the remote shell.)

@mxie91
Copy link
Contributor

mxie91 commented Oct 2, 2023 via email

@bsanders
Copy link
Author

bsanders commented Oct 2, 2023

@mxie91 note that the error specifically comes up when using -it ssh. Does it reproduce if you use that transport?

@rwmjones
Copy link
Member

rwmjones commented Oct 2, 2023

I posted a patch here based on Laszlo's diagnosis above:
https://listman.redhat.com/archives/libguestfs/2023-October/032745.html

rwmjones added a commit that referenced this issue Oct 2, 2023
@rwmjones
-it ssh: Double quote ssh command which tests remote file exists
22c5b98 
Double quoting was removed in
commit e2af12b ("input: -i vmx:
Remove support for openssh scp < 8.8", Nov 2021).  However it should
only have been removed from scp commands, not for this ssh command
where it is still required.

See: #35
Thanks: Laszlo Ersek for diagnosis and suggesting the fix
Reported-by: Bill Sanders
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
@bsanders
Copy link
Author

bsanders commented Oct 6, 2023

I just tested head of tree after that patch and on Debian 12 it works but fails at an scp step on Ubuntu 22.04.

That checks out based on discussions at the beginning of this issue. I'm able to switch to Debian 12, so that's good enough for me. Thanks for the patch :) I'm curious if it will be backported to 2.2.x?

rwmjones added a commit that referenced this issue Nov 10, 2023
@rwmjones
-it ssh: Double quote ssh command which tests remote file exists
450e6c7 
Double quoting was removed in
commit e2af12b ("input: -i vmx:
Remove support for openssh scp < 8.8", Nov 2021).  However it should
only have been removed from scp commands, not for this ssh command
where it is still required.

See: #35
Thanks: Laszlo Ersek for diagnosis and suggesting the fix
Reported-by: Bill Sanders
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(cherry picked from commit 22c5b98)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rwmjones @bsanders @lersek @mxie91