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

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Christian Lindig <christian.lindig@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	David Scott <dave@recoil.org>,
	Edwin Torok <edvin.torok@citrix.com>,
	Rob Hoes <Rob.Hoes@citrix.com>
Subject: Re: [PATCH v2 4/6] tools/oxenstored: Implement Domain.rebind_evtchn
Date: Thu, 1 Dec 2022 12:10:33 +0000	[thread overview]
Message-ID: <6c667e15-8a6c-753b-923d-fb5ea7839cc3@citrix.com> (raw)
In-Reply-To: <27ED9DA5-D63E-4F49-824A-74AD7149DA5C@citrix.com>

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