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...

~Andrew

Reply via email to