[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 rather than 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 (* Wait for a file to appear until a timeout. *)