Re: [PATCH v2 5/6] tools/oxenstored: Rework Domain evtchn handling to use port_pair

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 1 Dec 2022, at 14:22, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 01/12/2022 11:59, Christian Lindig wrote:
>>> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> 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.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
>>> CC: David Scott <dave@xxxxxxxxxx>
>>> CC: Edwin Torok <edvin.torok@xxxxxxxxxx>
>>> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>
>> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>
> 
> 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.


Yes:
* Hashtbl.add -> Hashtbl.replace
* Hashtbl.clear -> Hashtbl.reset

Using anything on the left is almost always an indication of a subtle bug (e.g. 
Hashtbl.clear won't release the memory used by the buckets, and the only time 
that is useful is if you'd immediately fill the hashtable with lots of elements 
again, really code should always use Hashtbl.reset but that only got introduced 
in OCaml 4.0.0, so older code won't have it).

And the use of Hashtbl.add can lead to "space leaks" (eventually OOM) unless 
one really knows what they are doing (i.e. there are only a finite number of 
add calls ever).

In XAPI we have a "quality gate" that counts the number of problematic 
functions/etc, and makes it a hard build time failure if any new usages are 
introduced (and we strive to reduce that to 0).
I don't think these 2 Hashtbl calls are there yet, but they probably should be.

Best regards,
--Edwin