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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

~Andrew