[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
Xenctrl.with_intf based on its undesirable behaviour of opening and
closing a Xenctrl connection with every invocation. This commit
re-introduces with_intf but with an updated behaviour: it maintains a
global Xenctrl connection which is opened upon first usage and kept
open. This handle can be obtained by clients using new functions
get_handle() and close_handle().

The main motivation of re-introducing with_intf is that otherwise
clients will have to implement this functionality individually.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl.ml  | 22 ++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl.mli | 10 ++++++++++
 2 files changed, 32 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 955dd92546..44857a2d1c 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -141,6 +141,28 @@ type handle
 external interface_open: unit -> handle = "stub_xc_interface_open"
 external interface_close: handle -> unit = "stub_xc_interface_close"
 
+let handle = ref None
+
+let get_handle () = !handle
+
+let close_handle () =
+	match !handle with
+	| Some h -> interface_close h
+	| None -> ()
+
+let with_intf f =
+	match !handle with
+	| Some h -> f h
+	| None ->
+		let h =
+			try interface_open () with
+			| e ->
+				let msg = Printexc.to_string e in
+				failwith ("failed to open xenctrl: "^msg)
+		in
+		handle := Some h;
+		f h
+
 external domain_create: handle -> domctl_create_config -> domid
        = "stub_xc_domain_create"
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index eeed24fa96..b31d7fefe5 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -109,6 +109,16 @@ type handle
 external interface_open : unit -> handle = "stub_xc_interface_open"
 external interface_close : handle -> unit = "stub_xc_interface_close"
 
+(** [with_intf f] runs [f] with a global handle that is opened on demand
+ * and kept open. A client should use either [interface_open]
+ * or [with_intf] but not mix both.
+ *)
+val with_intf : (handle -> 'a) -> 'a
+(** [get_handle] returns the global handle used by [with_intf] *)
+val get_handle: unit -> handle option
+(** [close handle] closes the handle maintained by [with_intf] *)
+val close_handle: unit -> unit
+
 external domain_create : handle -> domctl_create_config -> domid
   = "stub_xc_domain_create"
 external domain_sethandle : handle -> domid -> string -> unit = "stub_xc_domain_sethandle"
-- 
2.19.1


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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Ian Jackson]

Christian Lindig writes ("[Xen-devel] [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper"):
> Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
> Xenctrl.with_intf based on its undesirable behaviour of opening and
> closing a Xenctrl connection with every invocation. This commit
> re-introduces with_intf but with an updated behaviour: it maintains a
> global Xenctrl connection which is opened upon first usage and kept
> open. This handle can be obtained by clients using new functions
> get_handle() and close_handle().
...
> +let handle = ref None
> +
> +let get_handle () = !handle
> +
> +let close_handle () =
> +	match !handle with
> +	| Some h -> interface_close h
> +	| None -> ()

The semantics of close_handle are strange.  After it is used, handle
remains Some but is broken.

I think it would be better to have close_handle set handle to None,
and write in the docs that it is forbidden to call close_handle within
with_intf.  (Would it be possible to detect such a mistake?)

For that matter, why is close_handle needed at all ?

Regards,
Ian.

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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

> On 30 Oct 2018, at 16:48, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> I think it would be better to have close_handle set handle to None,
> and write in the docs that it is forbidden to call close_handle within
> with_intf. 

You are right - close_handle should set the handle to None in addition.

> For that matter, why is close_handle needed at all ?

I mostly added it for symmetry and it is needed to set the handle to None (see above) 
because interface_close() can’t do it.

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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Ian Jackson]

Christian Lindig writes ("Re: [Xen-devel] [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper"):
> You are right - close_handle should set the handle to None in addition.
> 
> > For that matter, why is close_handle needed at all ?
> 
> I mostly added it for symmetry and it is needed to set the handle to None (see above) 
> because interface_close() can’t do it.

Well, yes, but who would ever call it ?  It's not safe to use except
at the toplevel (whatever that means, but probably outside any
with_intf) because elsewhere you don't know whose work you're
sabotaging.

Which reminds me: why do you say it is wrong to use both
interface_open and with_intf ?  It's clumsy and probably a mistake but
it should function correctly I think.

Ian.

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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

> On 30 Oct 2018, at 17:01, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> Well, yes, but who would ever call it ?  It's not safe to use except
> at the toplevel (whatever that means, but probably outside any
> with_intf) because elsewhere you don't know whose work you're
> sabotaging.
> 
> Which reminds me: why do you say it is wrong to use both
> interface_open and with_intf ?  It's clumsy and probably a mistake but
> it should function correctly I think.
> 

I agree that it would not harm but conceptually I believe you 
should manage this explicitly using interface_{open/close} or implicitly 
using with_intf. I would use an exit handler to close this global handle
but it would be safe to never close it and to let the process terminate.

I said “should not” rather than “must not” for that reason. Maybe that is
subtle.

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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Ian Jackson]

Christian Lindig writes ("Re: [Xen-devel] [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper"):
> I agree that it would not harm but conceptually I believe you 
> should manage this explicitly using interface_{open/close} or implicitly 
> using with_intf. I would use an exit handler to close this global handle
> but it would be safe to never close it and to let the process terminate.

Right, but for example you might be in transition from one pattern to
the other.  That is not forbidden.

Ian.

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

[PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

This patch i updated:

* set the global handle to None
* updated documentation



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

[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
Xenctrl.with_intf based on its undesirable behaviour of opening and
closing a Xenctrl connection with every invocation. This commit
re-introduces with_intf but with an updated behaviour: it maintains a
global Xenctrl connection which is opened upon first usage and kept
open. This handle can be obtained by clients using new functions
get_handle() and close_handle().

The main motivation of re-introducing with_intf is that otherwise
clients will have to implement this functionality individually.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl.ml  | 22 ++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl.mli | 10 ++++++++++
 2 files changed, 32 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 955dd92546..a57130a3c3 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -141,6 +141,28 @@ type handle
 external interface_open: unit -> handle = "stub_xc_interface_open"
 external interface_close: handle -> unit = "stub_xc_interface_close"
 
+let handle = ref None
+
+let get_handle () = !handle
+
+let close_handle () =
+	match !handle with
+	| Some h -> handle := None; interface_close h
+	| None -> ()
+
+let with_intf f =
+	match !handle with
+	| Some h -> f h
+	| None ->
+		let h =
+			try interface_open () with
+			| e ->
+				let msg = Printexc.to_string e in
+				failwith ("failed to open xenctrl: "^msg)
+		in
+		handle := Some h;
+		f h
+
 external domain_create: handle -> domctl_create_config -> domid
        = "stub_xc_domain_create"
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index eeed24fa96..0d2ae34373 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -109,6 +109,16 @@ type handle
 external interface_open : unit -> handle = "stub_xc_interface_open"
 external interface_close : handle -> unit = "stub_xc_interface_close"
 
+(** [with_intf f] runs [f] with a global handle that is opened on demand
+ * and kept open. Conceptually, a client should use either
+ * interface_open and interface_close or with_intf although mixing both
+ * is possible *)
+val with_intf : (handle -> 'a) -> 'a
+(** [get_handle] returns the global handle used by [with_intf] *)
+val get_handle: unit -> handle option
+(** [close handle] closes the handle maintained by [with_intf] *)
+val close_handle: unit -> unit
+
 external domain_create : handle -> domctl_create_config -> domid
   = "stub_xc_domain_create"
 external domain_sethandle : handle -> domid -> string -> unit = "stub_xc_domain_sethandle"
-- 
2.19.1


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

Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]

[Ian Jackson]

Christian Lindig writes ("[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper"):
> Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
> Xenctrl.with_intf based on its undesirable behaviour of opening and
> closing a Xenctrl connection with every invocation. This commit
> re-introduces with_intf but with an updated behaviour: it maintains a
> global Xenctrl connection which is opened upon first usage and kept
> open. This handle can be obtained by clients using new functions
> get_handle() and close_handle().

Thanks, I like the changes you made.

I still think

> +(** [close handle] closes the handle maintained by [with_intf] *)
> +val close_handle: unit -> unit

this needs a stronger warning against anyone ever calling it, or a
clearer explanation of the consquences (whose scope is very broad).

Ian.

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

Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]

[Christian Lindig]

On 31/10/18 15:41, Ian Jackson wrote:
> this needs a stronger warning against anyone ever calling it, or a
> clearer explanation of the consquences (whose scope is very broad).

The consequence is that the global handle is closed but it would be 
opened again if
one called with_intf again. So I'm not sure that this would be 
dangerous. Maybe a warning
that is should be if at all called before process termination? I'm hapy 
to make such a change.

-- C

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

Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]

[Ian Jackson]

Christian Lindig writes ("Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]"):
> On 31/10/18 15:41, Ian Jackson wrote:
> > this needs a stronger warning against anyone ever calling it, or a
> > clearer explanation of the consquences (whose scope is very broad).
> 
> The consequence is that the global handle is closed but it would be 
> opened again if
> one called with_intf again. So I'm not sure that this would be 
> dangerous. Maybe a warning
> that is should be if at all called before process termination? I'm hapy 
> to make such a change.

 with_intf
    do some stuff with get_handle (1)
    spong
    do some stuff with get_handle (2)

 let spong = with_intf
    do some stuff with handle
    close_handle

(forgive my syntax but I hope you get the idea).  This will crash in
(2)'s call to get_handle.

IMO the bug is that spong calls close_handle which has a distant
destructive action, without being able to know that it is safe to do
so.

Ian.

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

Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]

[Christian Lindig]

On 31/10/18 16:01, Ian Jackson wrote:
> IMO the bug is that spong calls close_handle which has a distant
> destructive action, without being able to know that it is safe to do
> so.
I will add add more words or caution. Thanks for pointing it out.

-- C

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

Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]

[Ian Jackson]

Christian Lindig writes ("Re: [PATCH v2 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper [and 1 more messages]"):
> On 31/10/18 16:01, Ian Jackson wrote:
> > IMO the bug is that spong calls close_handle which has a distant
> > destructive action, without being able to know that it is safe to do
> > so.
> 
> I will add add more words or caution. Thanks for pointing it out.

YW, thank you.

Ian.

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

[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Christian Lindig]

Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
Xenctrl.with_intf based on its undesirable behaviour of opening and
closing a Xenctrl connection with every invocation. This commit
re-introduces with_intf but with an updated behaviour: it maintains a
global Xenctrl connection which is opened upon first usage and kept
open. This handle can be obtained by clients using new functions
get_handle() and close_handle().

The main motivation of re-introducing with_intf is that otherwise
clients will have to implement this functionality individually.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl.ml  | 22 ++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl.mli | 13 +++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 955dd92546..a57130a3c3 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -141,6 +141,28 @@ type handle
 external interface_open: unit -> handle = "stub_xc_interface_open"
 external interface_close: handle -> unit = "stub_xc_interface_close"
 
+let handle = ref None
+
+let get_handle () = !handle
+
+let close_handle () =
+	match !handle with
+	| Some h -> handle := None; interface_close h
+	| None -> ()
+
+let with_intf f =
+	match !handle with
+	| Some h -> f h
+	| None ->
+		let h =
+			try interface_open () with
+			| e ->
+				let msg = Printexc.to_string e in
+				failwith ("failed to open xenctrl: "^msg)
+		in
+		handle := Some h;
+		f h
+
 external domain_create: handle -> domctl_create_config -> domid
        = "stub_xc_domain_create"
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index eeed24fa96..476bbecb90 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -109,6 +109,19 @@ type handle
 external interface_open : unit -> handle = "stub_xc_interface_open"
 external interface_close : handle -> unit = "stub_xc_interface_close"
 
+(** [with_intf f] runs [f] with a global handle that is opened on demand
+ * and kept open. Conceptually, a client should use either
+ * interface_open and interface_close or with_intf although mixing both
+ * is possible *)
+val with_intf : (handle -> 'a) -> 'a
+(** [get_handle] returns the global handle used by [with_intf] *)
+val get_handle: unit -> handle option
+(** [close handle] closes the handle maintained by [with_intf]. This
+ * should only be closed before process exit. It must not be called from
+ * a function called directly or indirectly by with_intf as this
+ * would invalidate the handle that with_intf passes to its argument. *)
+val close_handle: unit -> unit
+
 external domain_create : handle -> domctl_create_config -> domid
   = "stub_xc_domain_create"
 external domain_sethandle : handle -> domid -> string -> unit = "stub_xc_domain_sethandle"
-- 
2.19.1


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

Re: [PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper

[Ian Jackson]

Christian Lindig writes ("[PATCH 1/1] tools/ocaml: Re-introduce Xenctrl.with_intf wrapper"):
> Commit 81946a73dc975a7dafe9017a8e61d1e64fdbedbf removed
> Xenctrl.with_intf based on its undesirable behaviour of opening and
> closing a Xenctrl connection with every invocation. This commit
> re-introduces with_intf but with an updated behaviour: it maintains a
> global Xenctrl connection which is opened upon first usage and kept
> open. This handle can be obtained by clients using new functions
> get_handle() and close_handle().
> 
> The main motivation of re-introducing with_intf is that otherwise
> clients will have to implement this functionality individually.

Thanks.

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

I will apply this patch and your other one.

Ian.

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