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

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.

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.

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