Re: [PATCH v2 4/6] tools/oxenstored: Implement Domain.rebind_evtchn

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 1 Dec 2022, at 12:10, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 01/12/2022 11:20, Christian Lindig wrote:
>> 
>>> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
>>> 
>>> Generally speaking, the event channel local/remote port is fixed for the
>>> lifetime of the associated domain object.  The exception to this is a
>>> secondary XS_INTRODUCE (defined to re-bind to a new event channel) which 
>>> pokes
>>> around at the domain object's internal state.
>>> 
>>> We need to refactor the evtchn handling to support live update, so start by
>>> moving the relevant manipulation into Domain.
>>> 
>>> No practical change.
>>> 
>>> 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.
> 
>> The code makes changes around if-expressions and it is easy to get mislead 
>> by indentation which parts are covered by an if and which are not in the 
>> presence of sequential code. I would be more confident about this with 
>> automatic formatting (but also believe this is correct).
> 
> I can keep the being/end if you'd prefer.
> 
> Looking at the end result, it would actually shrink the patch, so is
> probably worth doing anyway for clarity.  The net result is:
> 
> diff --git a/tools/ocaml/xenstored/process.ml
> b/tools/ocaml/xenstored/process.ml
> index b2973aca2a82..1c80e7198dbe 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -569,8 +569,7 @@ let do_introduce con t domains cons data =
>                         let edom = Domains.find domains domid in
>                         if (Domain.get_mfn edom) = mfn &&
> (Connections.find_domain cons domid) != con then begin
>                                 (* Use XS_INTRODUCE for recreating the
> xenbus event-channel. *)
> -                               edom.remote_port <- remote_port;
> -                               Domain.bind_interdomain edom;
> +                               Domain.rebind_evtchn edom remote_port;
>                         end;
>                         edom
>                 else try
> 
> I'm happy to adjust on commit.
> 
> When I started this, I tried rearranging things to avoid the "if exists
> then find" pattern, but quickly got into a mess, then realised that this
> is (almost) a dead logic path... I've got no idea why this is supported;
> looking through history, I can't find a case where a redundant
> XS_INTRODUCE was ever used, but its a common behaviour between C and O
> so there was clearly some reason...


Currently the soft reset code in xenopsd uses it, but as you say there must've 
been another reason too (the soft reset code is a lot more recent than this).

Best regards,
--Edwin