On 01/12/2022 11:26, Christian Lindig wrote: >> On 30 Nov 2022, at 16:54, Andrew Cooper <andrew.coop...@citrix.com> wrote: >> >> This will make the logic clearer when we plumb local_port through these >> functions. >> >> While changing this, simplify the construct in Domains.create0 to separate >> the >> remote port handling from the interface. >> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >> --- >> CC: Christian Lindig <christian.lin...@citrix.com> >> CC: David Scott <d...@recoil.org> >> CC: Edwin Torok <edvin.to...@citrix.com> >> CC: Rob Hoes <rob.h...@citrix.com> > Acked-by: Christian Lindig <christian.lin...@citrix.com>
Thanks. >> diff --git a/tools/ocaml/xenstored/domains.ml >> b/tools/ocaml/xenstored/domains.ml >> index 17fe2fa25772..26018ac0dd3d 100644 >> --- a/tools/ocaml/xenstored/domains.ml >> +++ b/tools/ocaml/xenstored/domains.ml >> @@ -133,18 +133,16 @@ let xenstored_kva = ref "" >> let xenstored_port = ref "" >> >> let create0 doms = >> - let port, interface = >> - ( >> - let port = Utils.read_file_single_integer >> !xenstored_port >> - and fd = Unix.openfile !xenstored_kva >> - [ Unix.O_RDWR ] 0o600 in >> - let interface = Xenmmap.mmap fd Xenmmap.RDWR >> Xenmmap.SHARED >> - (Xenmmap.getpagesize()) 0 in >> - Unix.close fd; >> - port, interface >> - ) >> - in >> - let dom = Domain.make 0 Nativeint.zero port interface doms.eventchn in >> + let remote_port = Utils.read_file_single_integer !xenstored_port in >> + >> + let interface = >> + let fd = Unix.openfile !xenstored_kva [ Unix.O_RDWR ] 0o600 in >> + let interface = Xenmmap.mmap fd Xenmmap.RDWR Xenmmap.SHARED >> (Xenmmap.getpagesize()) 0 in > Can we be sure that this never throws an exception such that the close can't > be missed? Otherwise a Fun.protect (or equivalent) should be used. This mess also depends on !xenstored_port and !xenstored_kva morphing into something other than "" before Domain.create0 is called. But this logic is also the penultimate unstable ABI in oxenstored, and will be removed fully when we can bind /dev/xen/gntdev for Ocaml and replace the foreign mapping with "map grant 1" (also removing this as a special case difference between dom0 and all other domains.) So I'm tempted to argue that I'm not actually changing the behaviour here, and it's not worth fixing up logic this fragile when we're intending to replace it anyway. Edvin has patches IIRC, but they need rebasing. ~Andrew
