Failed to connect to '/tmp/v2v.sKlulY/in0': Permission denied

Failed to connect socket to '/var/run/libvirt/virtqemud-sock-ro': Connection refused

On Thu, Jun 29, 2023 at 02:34:42PM +0200, Laszlo Ersek wrote: > Currently "chown_for_libvirt_rhbz_1045069" is best effort; if it fails, we > suppress the exception (we log it in verbose mode only, even). > > That's not proved helpful: it almost certainly leads to later errors, but > those errors are less clear than the original (suppressed) exception. > Namely, the user sees something like > >> Failed to connect to '/tmp/v2v.sKlulY/in0': Permission denied > > rather than > >> Failed to connect socket to '/var/run/libvirt/virtqemud-sock-ro': >> Connection refused > > So just allow the exception to propagate outwards. > > And then, now that "chown_for_libvirt_rhbz_1045069" will be able to fail, > hoist the call to "On_exit.rm_rf" before the call to > "chown_for_libvirt_rhbz_1045069", after creating the v2v temporary > directory. In the current order, if "chown_for_libvirt_rhbz_1045069" threw > an exception, then we'd leak the temp dir in the filesystem. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2182024 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > > Notes: > v2: > > - new patch > > lib/utils.mli | 5 +--- > lib/utils.ml | 26 ++++++++------------ > 2 files changed, 11 insertions(+), 20 deletions(-) > > diff --git a/lib/utils.mli b/lib/utils.mli > index cf88a467fd54..391a2a351ec7 100644 > --- a/lib/utils.mli > +++ b/lib/utils.mli > @@ -67,10 +67,7 @@ val chown_for_libvirt_rhbz_1045069 : string -> unit > to root-owned files and directories. To fix this, provide > a function to chown things we might need to qemu:root so > qemu can access them. Note that root normally ignores > - permissions so can still access the resource. > - > - This is best-effort. If something fails then we carry > - on and hope for the best. *) > + permissions so can still access the resource. *) > > val error_if_no_ssh_agent : unit -> unit > > diff --git a/lib/utils.ml b/lib/utils.ml > index 174c01b1e92f..7d69c9e0d177 100644 > --- a/lib/utils.ml > +++ b/lib/utils.ml > @@ -149,21 +149,15 @@ let backend_is_libvirt () = > > let rec chown_for_libvirt_rhbz_1045069 file = > let running_as_root = Unix.geteuid () = 0 in > - if running_as_root && backend_is_libvirt () then ( > - try > - let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in > - let uid = > - if String.is_prefix user "+" then > - int_of_string (String.sub user 1 (String.length user - 1)) > - else > - (Unix.getpwnam user).pw_uid in > - debug "setting owner of %s to %d:root" file uid; > - Unix.chown file uid 0 > - with > - | exn -> (* Print exception, but continue. *) > - debug "could not set owner of %s: %s" > - file (Printexc.to_string exn) > - ) > + if running_as_root && backend_is_libvirt () then > + let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in > + let uid = > + if String.is_prefix user "+" then > + int_of_string (String.sub user 1 (String.length user - 1)) > + else > + (Unix.getpwnam user).pw_uid in > + debug "setting owner of %s to %d:root" file uid; > + Unix.chown file uid 0 Hmm, doesn't this need parens around the 'then' clause?

> (* Get the local user that libvirt uses to run qemu when we are > * running as root. This is returned as an optional string > @@ -205,8 +199,8 @@ let error_if_no_ssh_agent () = > (* Create the directory containing inX and outX sockets. *) > let create_v2v_directory () = > let d = Mkdtemp.temp_dir "v2v." in > - chown_for_libvirt_rhbz_1045069 d; > On_exit.rm_rf d; > + chown_for_libvirt_rhbz_1045069 d; > d Yes, as you explain in the commit message this hunk is needed (and dependent) because the chown might now fail. Rich.

On Thu, Jun 29, 2023 at 05:39:34PM +0200, Laszlo Ersek wrote: > On 6/29/23 14:54, Richard W.M. Jones wrote: >> On Thu, Jun 29, 2023 at 02:34:42PM +0200, Laszlo Ersek wrote: >>> Currently "chown_for_libvirt_rhbz_1045069" is best effort; if it fails, we >>> suppress the exception (we log it in verbose mode only, even). >>> >>> That's not proved helpful: it almost certainly leads to later errors, but >>> those errors are less clear than the original (suppressed) exception. >>> Namely, the user sees something like >>> >>>> Failed to connect to '/tmp/v2v.sKlulY/in0': Permission denied >>> >>> rather than >>> >>>> Failed to connect socket to '/var/run/libvirt/virtqemud-sock-ro': >>>> Connection refused >>> >>> So just allow the exception to propagate outwards. >>> >>> And then, now that "chown_for_libvirt_rhbz_1045069" will be able to fail, >>> hoist the call to "On_exit.rm_rf" before the call to >>> "chown_for_libvirt_rhbz_1045069", after creating the v2v temporary >>> directory. In the current order, if "chown_for_libvirt_rhbz_1045069" threw >>> an exception, then we'd leak the temp dir in the filesystem. >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2182024 >>> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >>> --- >>> >>> Notes: >>> v2: >>> >>> - new patch >>> >>> lib/utils.mli | 5 +--- >>> lib/utils.ml | 26 ++++++++------------ >>> 2 files changed, 11 insertions(+), 20 deletions(-) >>> >>> diff --git a/lib/utils.mli b/lib/utils.mli >>> index cf88a467fd54..391a2a351ec7 100644 >>> --- a/lib/utils.mli >>> +++ b/lib/utils.mli >>> @@ -67,10 +67,7 @@ val chown_for_libvirt_rhbz_1045069 : string -> unit >>> to root-owned files and directories. To fix this, provide >>> a function to chown things we might need to qemu:root so >>> qemu can access them. Note that root normally ignores >>> - permissions so can still access the resource. >>> - >>> - This is best-effort. If something fails then we carry >>> - on and hope for the best. *) >>> + permissions so can still access the resource. *) >>> >>> val error_if_no_ssh_agent : unit -> unit >>> >>> diff --git a/lib/utils.ml b/lib/utils.ml >>> index 174c01b1e92f..7d69c9e0d177 100644 >>> --- a/lib/utils.ml >>> +++ b/lib/utils.ml >>> @@ -149,21 +149,15 @@ let backend_is_libvirt () = >>> >>> let rec chown_for_libvirt_rhbz_1045069 file = >>> let running_as_root = Unix.geteuid () = 0 in >>> - if running_as_root && backend_is_libvirt () then ( >>> - try >>> - let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in >>> - let uid = >>> - if String.is_prefix user "+" then >>> - int_of_string (String.sub user 1 (String.length user - 1)) >>> - else >>> - (Unix.getpwnam user).pw_uid in >>> - debug "setting owner of %s to %d:root" file uid; >>> - Unix.chown file uid 0 >>> - with >>> - | exn -> (* Print exception, but continue. *) >>> - debug "could not set owner of %s: %s" >>> - file (Printexc.to_string exn) >>> - ) >>> + if running_as_root && backend_is_libvirt () then >>> + let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in >>> + let uid = >>> + if String.is_prefix user "+" then >>> + int_of_string (String.sub user 1 (String.length user - 1)) >>> + else >>> + (Unix.getpwnam user).pw_uid in >>> + debug "setting owner of %s to %d:root" file uid; >>> + Unix.chown file uid 0 >> >> Hmm, doesn't this need parens around the 'then' clause? > > Ding ding ding, it does not :) > > I'm happy you asked. (I was so frustrated to discover this *once again* > that I almost sent a separate tirade about it to the list! So I guess > now I'll use the opportunity...) > > The short answer is that, if "Unix.chown" were out of the scope of the > "then", then it would also be out of the scope of "uid", and so it > wouldn't compile. Well, unless there happened to be a "uid" symbol somewhere in the outer scope (or we added one later), or in any "open"-d module. Tirade aside, not having the parentheses makes this code fragile. eg. It'll break unpredictably if someone inserts a statement after the debug command.

> But the long answer is more interesting: > > # open Printf;; > > # if false then printf "1\n"; printf "2\n";; > 2 > - : unit = () > > And then compare: > > # if false then let () = () in printf "1\n"; printf "2\n";; > - : unit = () > > When we have "then" and a semiclon ";" but *no* "let/in", then the > "then" binds more strongly than the semicolon. > > When we have a "then", a "let/in", and a semicolon ";", then the > semicolon binds more strongly than the "in", *and* the "let/in" binds > more strongly than the "then". Transitively, the semicolon binds more > strongly than the "then". > > In other words, the "let/in" isn't just *inserted* between the "then" > and the semicolon ";", preserving their relative binding strengths -- > their relative binding strengths are *reversed*, without any explicit > parens! > > This is *insane* in OCaml. That's a bit surprising, and I've been programming in ML for about 30 years. However just like in C (which has another variant of the same issue) putting in brackets helps to clarify what you really mean. > See also: > > - the following article: > http://ocamlverse.net/content/faq_if_semicolon.html > > - the following video: > https://www.youtube.com/watch?v=aj0bpOyv7Gs > > In particular, the video claims at about 1:32 that the sequencing with > the semicolon > > e1; e2 > > is just syntactic sugar for > > let () = e1 in > e2 > > Well, that claim is wrong. The purported equivalence holds only for > > ( e1; e2 ) > > (note the parens!), not for the naked > > e1; e2 > > Consider our original > > if false then printf "1\n"; printf "2\n" [1] > > versus > > if false then (printf "1\n"; printf "2\n") [2] > > The latter [2] *indeed* corresponds to: > > if false then > let () = printf "1\n" in > printf "2\n" > > But what is the *former* [1], with the syntactic sugar removed? Well, it > turns out: > > let () = if false then printf "1\n" in > printf "2\n" > > Crazy. > > > In the patch, I removed the parens because they'd make the wrong > impression. They'd imply they made a difference relative to the default > bindings. But that's not the case. It's not the *presence* of a closing > paren *after* the Unix.chown call that matters, rather it's the > *absence* of a closing paren *before* Unix.chown -- because the latter > would change behavior: > > let rec chown_for_libvirt_rhbz_1045069 file = > let running_as_root = Unix.geteuid () = 0 in > if running_as_root && backend_is_libvirt () then ( > let user = Option.value ~default:"qemu" (libvirt_qemu_user ()) in > let uid = > if String.is_prefix user "+" then > int_of_string (String.sub user 1 (String.length user - 1)) > else > (Unix.getpwnam user).pw_uid in > debug "setting owner of %s to %d:root" file uid > ); > Unix.chown file uid 0 > > (and this highlights how we'd be referencing "uid" out of scope). > >> >>> (* Get the local user that libvirt uses to run qemu when we are >>> * running as root. This is returned as an optional string >>> @@ -205,8 +199,8 @@ let error_if_no_ssh_agent () = >>> (* Create the directory containing inX and outX sockets. *) >>> let create_v2v_directory () = >>> let d = Mkdtemp.temp_dir "v2v." in >>> - chown_for_libvirt_rhbz_1045069 d; >>> On_exit.rm_rf d; >>> + chown_for_libvirt_rhbz_1045069 d; >>> d >> >> Yes, as you explain in the commit message this hunk is needed (and >> dependent) because the chown might now fail. >> >> Rich. >> > > Laszlo Rich.

It has frequently tripped us up that on RHEL / Fedora, installing the right set of libvirt RPMs (such as the one pulled in by "libvirt-daemon-kvm") does not result in an immediately running libvirt system instance. Document the need, and the simplest method, for starting libvirt up manually. Thanks: Daniel Berrangé Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2182024 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: v2: - rewrite using the language suggested by Rich context:-U12 docs/virt-v2v.pod | 23 +++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/virt-v2v.pod b/docs/virt-v2v.pod index 4d2f241ad723..ac15108ae332 100644 --- a/docs/virt-v2v.pod +++ b/docs/virt-v2v.pod @@ -250,24 +250,26 @@ metadata. virt-v2v tries to guess the best default metadata. This is usually adequate but you can get finer control (eg. of memory and vCPUs) by using I<-i libvirtxml> instead. Only guests that use a single disk can be imported this way. =item B<-i> B<libvirt> Set the input method to I<libvirt>. This is the default. In this mode you have to specify a libvirt guest name or UUID on the command line. You may also specify a libvirt connection URI (see I<-ic>). +See L</Starting the libvirt system instance> below. + =item B<-i> B<libvirtxml> Set the input method to I<libvirtxml>. In this mode you have to pass a libvirt XML file on the command line. This file is read in order to get metadata about the source guest (such as its name, amount of memory), and also to locate the input disks. See L</Minimal XML for -i libvirtxml option> below. =item B<-i> B<local> This is the same as I<-i disk>. @@ -461,25 +463,26 @@ and guest metadata is created in the associated YAML file: /dir/name.yaml where C<name> is the guest name. =item B<-o> B<libvirt> Set the output method to I<libvirt>. This is the default. In this mode, the converted guest is created as a libvirt guest. You may also specify a libvirt connection URI (see I<-oc>). -See L<virt-v2v-output-local(1)>. +See L</Starting the libvirt system instance> below, and +L<virt-v2v-output-local(1)>. =item B<-o> B<local> Set the output method to I<local>. In this mode, the converted guest is written to a local directory specified by I<-os /dir> (the directory must exist). The converted guest’s disks are written as: /dir/name-sda /dir/name-sdb [etc] @@ -1373,24 +1376,26 @@ manually change ownership after virt-v2v has finished. =item Writing to libvirt When using I<-o libvirt>, you may need to run virt-v2v as root so that it can write to the libvirt system instance (ie. C<qemu:///system>) and to the default location for disk images (usually F</var/lib/libvirt/images>). You can avoid this by setting up libvirt connection authentication, see L<http://libvirt.org/auth.html>;. Alternatively, use I<-oc qemu:///session>, which will write to your per-user libvirt instance. +See also L</Starting the libvirt system instance>. + =item Writing to Openstack Because of how Cinder volumes are presented as F</dev> block devices, using I<-o openstack> normally requires that virt-v2v is run as root. =item Writing to Glance This does I<not> need root (in fact it probably won’t work), but may require either a special user and/or for you to source a script that sets authentication environment variables. Consult the Glance documentation. @@ -1521,24 +1526,40 @@ displayed to the user. The calling program should treat messages sent to stderr as error messages. In addition, virt-v2v exits with a non-zero status code if there was a fatal error. =back Virt-v2v E<le> 0.9.1 did not support the I<--machine-readable> option at all. The option was added when virt-v2v was rewritten in 2014. It is possible to specify a format string for controlling the output; see L<guestfs(3)/ADVANCED MACHINE READABLE OUTPUT>. +=head2 Starting the libvirt system instance + + Failed to connect socket to '/var/run/libvirt/virtqemud-sock': No such file or directory + Failed to connect socket to '/var/run/libvirt/virtqemud-sock-ro': Connection refused + +If you have just installed libvirt and virt-v2v, then you may see the +errors above. This is caused by libvirt daemons that provide various +services not running straight after installation. (This may depend on +your distribution and vendor presets). + +To fix this on systemd-based distributions, do: + + systemctl isolate multi-user.target + +See also L<https://bugzilla.redhat.com/2182024>;. + =head1 FILES =over 4 =item F</usr/share/virtio-win> (Optional) If this directory is present, then virtio drivers for Windows guests will be found from this directory and installed in the guest during conversion.