On Thu, Jun 29, 2023 at 06:40:27PM +0200, Laszlo Ersek wrote:
> 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 <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?
> >>
> >> 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?

I think the parens are clearer.  Don't need a v3, thanks!

Rich.

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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Reply via email to