Re: [Libguestfs] [v2v PATCH v2 2/3] lib/utils: make "chown_for_libvirt_rhbz_1045069" fail hard

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

(* 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