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

On 6/29/23 17:48, Richard W.M. Jones wrote: > 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. Inserting another statement between the debug and the Unix.chown will not change a thing; they are all gobbled up by the innermost "in" just the same. There *is* some fragility indeed, but in the opposite direction -- if we add something after the "Unix.chown", and we don't want it to depend on the "if", then we certainly need the parens. Either way, I can restore the parens. Do you want me to submit a v3?

Laszlo > >> 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. >