[Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Igor Druzhinin]

C version of xenstored had this ability since 61aaed0d5 ("Allow
XS_INTRODUCE to be used for rebinding the xenstore evtchn.") from 2007.
Copy it as is to Ocaml version.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 tools/ocaml/xenstored/domain.ml  | 6 +++++-
 tools/ocaml/xenstored/process.ml | 8 +++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
index b0a01b0..aeb185f 100644
--- a/tools/ocaml/xenstored/domain.ml
+++ b/tools/ocaml/xenstored/domain.ml
@@ -23,9 +23,9 @@ type t =
 {
 	id: Xenctrl.domid;
 	mfn: nativeint;
-	remote_port: int;
 	interface: Xenmmap.mmap_interface;
 	eventchn: Event.t;
+	mutable remote_port: int;
 	mutable port: Xeneventchn.t option;
 	mutable bad_client: bool;
 	mutable io_credit: int; (* the rounds of ring process left to do, default is 0,
@@ -71,6 +71,10 @@ let notify dom = match dom.port with
 	Event.notify dom.eventchn port
 
 let bind_interdomain dom =
+	begin match dom.port with
+	| None -> ()
+	| Some port -> Event.unbind dom.eventchn port
+	end;
 	dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port);
 	debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port)
 
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 8a7e538..ff5c948 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -408,7 +408,13 @@ let do_introduce con _t domains cons data =
 		in
 	let dom =
 		if Domains.exist domains domid then
-			Domains.find domains domid
+			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 <- port;
+				Domain.bind_interdomain edom;
+			end;
+			edom
 		else try
 			let ndom = Domains.create domains domid mfn port in
 			Connections.add_domain cons ndom;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Christian Lindig]

> +			if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin

This should use <> instead of != because != is pointer inequality in OCaml. The parentheses are not strictly necessary because function application has precedence. So:

	if Domain.get_mfn edom = mfn && Connections.find_domain cons domid <> con then begin

— Christian

> On 19 Aug 2019, at 19:45, Igor Druzhinin <igor.druzhinin@citrix.com> wrote:
> 
> C version of xenstored had this ability since 61aaed0d5 ("Allow
> XS_INTRODUCE to be used for rebinding the xenstore evtchn.") from 2007.
> Copy it as is to Ocaml version.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> tools/ocaml/xenstored/domain.ml  | 6 +++++-
> tools/ocaml/xenstored/process.ml | 8 +++++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
> index b0a01b0..aeb185f 100644
> --- a/tools/ocaml/xenstored/domain.ml
> +++ b/tools/ocaml/xenstored/domain.ml
> @@ -23,9 +23,9 @@ type t =
> {
> 	id: Xenctrl.domid;
> 	mfn: nativeint;
> -	remote_port: int;
> 	interface: Xenmmap.mmap_interface;
> 	eventchn: Event.t;
> +	mutable remote_port: int;
> 	mutable port: Xeneventchn.t option;
> 	mutable bad_client: bool;
> 	mutable io_credit: int; (* the rounds of ring process left to do, default is 0,
> @@ -71,6 +71,10 @@ let notify dom = match dom.port with
> 	Event.notify dom.eventchn port
> 
> let bind_interdomain dom =
> +	begin match dom.port with
> +	| None -> ()
> +	| Some port -> Event.unbind dom.eventchn port
> +	end;
> 	dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port);
> 	debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port)
> 
> diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
> index 8a7e538..ff5c948 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -408,7 +408,13 @@ let do_introduce con _t domains cons data =
> 		in
> 	let dom =
> 		if Domains.exist domains domid then
> -			Domains.find domains domid
> +			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 <- port;
> +				Domain.bind_interdomain edom;
> +			end;
> +			edom
> 		else try
> 			let ndom = Domains.create domains domid mfn port in
> 			Connections.add_domain cons ndom;
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Igor Druzhinin]

On 20/08/2019 09:21, Christian Lindig wrote:
>> +			if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin
> 
> This should use <> instead of != because != is pointer inequality in OCaml. The parentheses are not strictly necessary because function application has precedence. So:
> 
> 	if Domain.get_mfn edom = mfn && Connections.find_domain cons domid <> con then begin
> 

But I actually want to compare pointers here - the idea is that the
connection object in the hashtable indexed by domid is not the same as
connection that we got XS_INTRODUCE message from. (see
tools/xenstore/xenstrored_domain.c)

Igor

>> On 19 Aug 2019, at 19:45, Igor Druzhinin <igor.druzhinin@citrix.com> wrote:
>>
>> C version of xenstored had this ability since 61aaed0d5 ("Allow
>> XS_INTRODUCE to be used for rebinding the xenstore evtchn.") from 2007.
>> Copy it as is to Ocaml version.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>> tools/ocaml/xenstored/domain.ml  | 6 +++++-
>> tools/ocaml/xenstored/process.ml | 8 +++++++-
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
>> index b0a01b0..aeb185f 100644
>> --- a/tools/ocaml/xenstored/domain.ml
>> +++ b/tools/ocaml/xenstored/domain.ml
>> @@ -23,9 +23,9 @@ type t =
>> {
>> 	id: Xenctrl.domid;
>> 	mfn: nativeint;
>> -	remote_port: int;
>> 	interface: Xenmmap.mmap_interface;
>> 	eventchn: Event.t;
>> +	mutable remote_port: int;
>> 	mutable port: Xeneventchn.t option;
>> 	mutable bad_client: bool;
>> 	mutable io_credit: int; (* the rounds of ring process left to do, default is 0,
>> @@ -71,6 +71,10 @@ let notify dom = match dom.port with
>> 	Event.notify dom.eventchn port
>>
>> let bind_interdomain dom =
>> +	begin match dom.port with
>> +	| None -> ()
>> +	| Some port -> Event.unbind dom.eventchn port
>> +	end;
>> 	dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port);
>> 	debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port)
>>
>> diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
>> index 8a7e538..ff5c948 100644
>> --- a/tools/ocaml/xenstored/process.ml
>> +++ b/tools/ocaml/xenstored/process.ml
>> @@ -408,7 +408,13 @@ let do_introduce con _t domains cons data =
>> 		in
>> 	let dom =
>> 		if Domains.exist domains domid then
>> -			Domains.find domains domid
>> +			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 <- port;
>> +				Domain.bind_interdomain edom;
>> +			end;
>> +			edom
>> 		else try
>> 			let ndom = Domains.create domains domid mfn port in
>> 			Connections.add_domain cons ndom;
>> -- 
>> 2.7.4
>>
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Christian Lindig]

> On 20 Aug 2019, at 11:45, Igor Druzhinin <igor.druzhinin@citrix.com> wrote:
> 
> On 20/08/2019 09:21, Christian Lindig wrote:
>>> +			if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin
>> 
>> This should use <> instead of != because != is pointer inequality in OCaml. The parentheses are not strictly necessary because function application has precedence. So:
>> 
>> 	if Domain.get_mfn edom = mfn && Connections.find_domain cons domid <> con then begin
>> 
> 
> But I actually want to compare pointers here - the idea is that the
> connection object in the hashtable indexed by domid is not the same as
> connection that we got XS_INTRODUCE message from. (see
> tools/xenstore/xenstrored_domain.c)

In your code, a != b is true, if a and b have identical structure but different addresses. I strongly suspect that two connection values should have different structure to be considered different, not just different addresses. When a <> b is true, it implies a != b. So using a <> b is safe(r). By using != you would rely on an invariant that every connection (con) exists only once and is never copied.

— Christian


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Igor Druzhinin]

On 20/08/2019 13:11, Christian Lindig wrote:
> 
> 
>> On 20 Aug 2019, at 11:45, Igor Druzhinin <igor.druzhinin@citrix.com> wrote:
>>
>> On 20/08/2019 09:21, Christian Lindig wrote:
>>>> +			if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin
>>>
>>> This should use <> instead of != because != is pointer inequality in OCaml. The parentheses are not strictly necessary because function application has precedence. So:
>>>
>>> 	if Domain.get_mfn edom = mfn && Connections.find_domain cons domid <> con then begin
>>>
>>
>> But I actually want to compare pointers here - the idea is that the
>> connection object in the hashtable indexed by domid is not the same as
>> connection that we got XS_INTRODUCE message from. (see
>> tools/xenstore/xenstrored_domain.c)
> 
> In your code, a != b is true, if a and b have identical structure but different addresses. I strongly suspect that two connection values should have different structure to be considered different, not just different addresses. When a <> b is true, it implies a != b. So using a <> b is safe(r). By using != you would rely on an invariant that every connection (con) exists only once and is never copied.
> 

Connection is a complex object (has various counters and internal
structures) and I don't think simple field by field comparison (is <>
just that in Ocaml?) is appropriate here.

con is passed into that function from xenstored.ml where it's extracted
from the same hashtable (Connections) so it's actually the exactly same
object. There are other examples in connections.ml where pointer
comparison is used to compare connections.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/oxenstored: port XS_INTRODUCE evtchn rebind function from cxenstored

[Christian Lindig]

> On 20 Aug 2019, at 13:45, Igor Druzhinin <igor.druzhinin@citrix.com> wrote:
> 
> con is passed into that function from xenstored.ml where it's extracted
> from the same hashtable (Connections) so it's actually the exactly same
> object. 

This comes down to what semantics you want. It is unfortunate that Connection.t is not abstract and its equality not well defined. Using != now relies on the fact that you only compare values stored in the same hash table.

— C

Acked-by: Christian Lindig <christian.lindig@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel