> On 1 Dec 2022, at 12:10, Andrew Cooper <andrew.coop...@citrix.com> wrote:
>
> On 01/12/2022 11:20, Christian Lindig wrote:
>>
>>> On 30 Nov 2022, at 16:54, Andrew Cooper <andrew.coop...@citrix.com> 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.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.
>
>> 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
