On 01/12/2022 11:59, Christian Lindig wrote: >> On 30 Nov 2022, at 16:54, Andrew Cooper <andrew.coop...@citrix.com> wrote: >> >> Inter-domain event channels are always a pair of local and remote ports. >> Right now the handling is asymmetric, caused by the fact that the evtchn is >> bound after the associated Domain object is constructed. >> >> First, move binding of the event channel into the Domain.make() constructor. >> This means the local port no longer needs to be an option. It also removes >> the final callers of Domain.bind_interdomain. >> >> Next, introduce a new port_pair type to encapsulate the fact that these two >> should be updated together, and replace the previous port and remote_port >> fields. This refactoring also changes the Domain.get_port interface >> (removing >> an option) so take the opportunity to name it get_local_port instead. >> >> Also, this fixes a use-after-free risk with Domain.close. Once the evtchn >> has >> been unbound, the same local port number can be reused for a different >> purpose, so explicitly invalidate the ports to prevent their accidental >> misuse >> in the future. >> >> This also cleans up some of the debugging, to always print a port pair. >> >> 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. > >> v2: >> * New >> --- >> tools/ocaml/xenstored/connections.ml | 9 +---- >> tools/ocaml/xenstored/domain.ml | 75 >> ++++++++++++++++++------------------ >> tools/ocaml/xenstored/domains.ml | 2 - >> 3 files changed, 39 insertions(+), 47 deletions(-) >> >> diff --git a/tools/ocaml/xenstored/connections.ml >> b/tools/ocaml/xenstored/connections.ml >> index 7d68c583b43a..a80ae0bed2ce 100644 >> --- a/tools/ocaml/xenstored/connections.ml >> +++ b/tools/ocaml/xenstored/connections.ml >> @@ -48,9 +48,7 @@ let add_domain cons dom = >> let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) >> (fun () -> Domain.notify dom) in >> let con = Connection.create xbcon (Some dom) in >> Hashtbl.add cons.domains (Domain.get_id dom) con; >> - match Domain.get_port dom with >> - | Some p -> Hashtbl.add cons.ports p con; >> - | None -> () >> + Hashtbl.add cons.ports (Domain.get_local_port dom) con > I would prefer Hashtbl.replace. Hashtbl.add shadows an existing binding which > becomes visible again after Hashtabl.remove. When we are sure that we only > have one binding per key, we should use replace instead of add. That's surprising behaviour. Presumably the add->replace suggestion applies the other hashtable here (cons.domains)? And possibly elsewhere too. ~Andrew
