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 <ler...@redhat.com> > --- > > 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. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
