[PATCH 1/2] make xen ocaml safe-strings compliant

[PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

Xen built with ocaml 4.06 gives errors such as
Error: This expression has type bytes but an expression was
 	expected of type string
as Byte and safe-strings which were introduced in 4.02 are the
default in 4.06.
This patch which is partly by Richard W.M. Jones of Red Hat
from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
fixes these issues.

Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
---
  tools/ocaml/libs/xb/xb.ml        |  6 +++---
  tools/ocaml/libs/xc/xenctrl.ml   |  2 +-
  tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
  tools/ocaml/xenstored/stdext.ml  |  2 +-
  tools/ocaml/xenstored/utils.ml   | 18 +++++++++---------
  5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 50944b5fd6..aa2cf98223 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -84,7 +84,7 @@ let read_mmap back con s len =

  let read con s len =
  	match con.backend with
-	| Fd backfd     -> read_fd backfd con s len
+	| Fd backfd     -> read_fd backfd con (Bytes.of_string s) len
  	| Xenmmap backmmap -> read_mmap backmmap con s len

  let write_fd back con s len =
@@ -98,7 +98,7 @@ let write_mmap back con s len =

  let write con s len =
  	match con.backend with
-	| Fd backfd     -> write_fd backfd con s len
+	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
  	| Xenmmap backmmap -> write_mmap backmmap con s len

  (* NB: can throw Reconnect *)
@@ -147,7 +147,7 @@ let input con =
  	| NoHdr (i, buf)      ->
  		(* we complete the partial header *)
  		if sz > 0 then
-			String.blit s 0 buf (Partial.header_size () - i) sz;
+			String.blit s 0 (Bytes.of_string buf) (Partial.header_size () - i) sz;
  		con.partial_in <- if sz = i then
  			HaveHdr (Partial.of_string buf) else NoHdr (i - sz, buf)
  	);
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 9116aa222c..31b13f4bbd 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -292,7 +292,7 @@ external marshall_core_header: core_header -> string = "stub_marshall_core_heade
  (* coredump *)
  let coredump xch domid fd =
  	let dump s =
-		let wd = Unix.write fd s 0 (String.length s) in
+		let wd = Unix.write fd (Bytes.of_string s) 0 (String.length s) in
  		if wd <> String.length s then
  			failwith "error while writing";
  		in
diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 0c0d03d0c4..d24abf8a3a 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -60,11 +60,11 @@ type logger =
  let truncate_line nb_chars line =
  	if String.length line > nb_chars - 1 then
  		let len = max (nb_chars - 1) 2 in
-		let dst_line = String.create len in
-		String.blit line 0 dst_line 0 (len - 2);
-		dst_line.[len-2] <- '.';
-		dst_line.[len-1] <- '.';
-		dst_line
+		let dst_line = Bytes.create len in
+		Bytes.blit_string line 0 dst_line 0 (len - 2);
+		Bytes.set dst_line (len-2) '.';
+		Bytes.set dst_line (len-1) '.';
+		Bytes.to_string dst_line
  	else line

  let log_rotate ref_ch log_file log_nb_files =
@@ -252,13 +252,13 @@ let string_of_access_type = function
  	*)

  let sanitize_data data =
-	let data = String.copy data in
-	for i = 0 to String.length data - 1
+	let data = Bytes.copy data in
+	for i = 0 to Bytes.length data - 1
  	do
-		if data.[i] = '\000' then
-			data.[i] <- ' '
+		if Bytes.get data i = '\000' then
+			Bytes.set data i ' '
  	done;
-	String.escaped data
+	String.escaped (Bytes.to_string data)

  let activate_access_log = ref true
  let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -291,7 +291,7 @@ let access_logging ~con ~tid ?(data="") ~level access_type =
  				let date = string_of_date() in
  				let tid = string_of_tid ~con tid in
  				let access_type = string_of_access_type access_type in
-				let data = sanitize_data data in
+				let data = sanitize_data (Bytes.of_string data) in
  				let prefix = prefix !access_log_destination date in
  				let msg = Printf.sprintf "%s %s %s %s" prefix tid access_type data in
  				logger.write ~level msg)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index b8a8fd00e1..d05155c97e 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
  		let pid = Unix.getpid () in
  		let buf = string_of_int pid ^ "\n" in
  		let len = String.length buf in
-		if Unix.write fd buf 0 len <> len
+		if Unix.write fd (Bytes.of_string buf) 0 len <> len
  		then failwith "pidfile_write failed";
  	)
  	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index e89c1aff04..c96def7bee 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -45,23 +45,23 @@ let get_hierarchy path =

  let hexify s =
  	let hexseq_of_char c = sprintf "%02x" (Char.code c) in
-	let hs = String.create (String.length s * 2) in
+	let hs = Bytes.create (String.length s * 2) in
  	for i = 0 to String.length s - 1
  	do
  		let seq = hexseq_of_char s.[i] in
-		hs.[i * 2] <- seq.[0];
-		hs.[i * 2 + 1] <- seq.[1];
+		Bytes.set hs (i * 2) seq.[0];
+		Bytes.set hs (i * 2 + 1) seq.[1];
  	done;
-	hs
+	Bytes.to_string hs

  let unhexify hs =
  	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
-	let s = String.create (String.length hs / 2) in
-	for i = 0 to String.length s - 1
+	let s = Bytes.create (String.length hs / 2) in
+	for i = 0 to Bytes.length s - 1
  	do
-		s.[i] <- char_of_hexseq hs.[i * 2] hs.[i * 2 + 1]
+		Bytes.set s i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
  	done;
-	s
+	Bytes.to_string s

  let trim_path path =
  	try
@@ -85,7 +85,7 @@ let create_unix_socket name =
  let read_file_single_integer filename =
  	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
  	let buf = String.make 20 (char_of_int 0) in
-	let sz = Unix.read fd buf 0 20 in
+	let sz = Unix.read fd (Bytes.of_string buf) 0 20 in
  	Unix.close fd;
  	int_of_string (String.sub buf 0 sz)

-- 
2.14.3

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Tue, Jan 30, 2018 at 10:55:47PM +0000, Michael Young wrote:
> Xen built with ocaml 4.06 gives errors such as
> Error: This expression has type bytes but an expression was
> 	expected of type string
> as Byte and safe-strings which were introduced in 4.02 are the
> default in 4.06.
> This patch which is partly by Richard W.M. Jones of Red Hat
> from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
> fixes these issues.
> 
> Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> Reviewed-by: Christian Lindig <christian.lindig@citrix.com>

Strangely this doesn't apply to staging. And after examining the
downloaded patch I'm not sure if my mail client is acting up. Do you
have a git branch that I can pull from?

Wei.

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Tue, 6 Feb 2018, Wei Liu wrote:

> On Tue, Jan 30, 2018 at 10:55:47PM +0000, Michael Young wrote:
>> Xen built with ocaml 4.06 gives errors such as
>> Error: This expression has type bytes but an expression was
>> 	expected of type string
>> as Byte and safe-strings which were introduced in 4.02 are the
>> default in 4.06.
>> This patch which is partly by Richard W.M. Jones of Red Hat
>> from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
>> fixes these issues.
>>
>> Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
>> Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
>
> Strangely this doesn't apply to staging. And after examining the
> downloaded patch I'm not sure if my mail client is acting up. Do you
> have a git branch that I can pull from?

The patch needed to be reduced as one of the sections being patched was 
removed by a recent patch. I am attaching the revised patch as a file 
in case there was also an email formatting issue.

 	Michael Young

[-- Attachment #2: Type: text/plain, Size: 6150 bytes --]

From 247cea9b587baafaf80bcc5b44bc68defb4efa26 Mon Sep 17 00:00:00 2001
From: Michael Young <m.a.young@durham.ac.uk>
Date: Tue, 6 Feb 2018 21:27:23 +0000
Subject: [PATCH 1/2 v2] make xen ocaml safe-strings compliant

Xen built with ocaml 4.06 gives errors such as
Error: This expression has type bytes but an expression was
        expected of type string
as Byte and safe-strings which were introduced in 4.02 are the
default in 4.06.
This patch which is mostly by Richard W.M. Jones of Red Hat
from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
fixes these issues.

v2: drop tools/ocaml/libs/xc/xenctrl.ml from the patch as the
affected code was removed by commit d933f1a53c06002351c1e36d40615e40bd4bf6af
tools/ocaml: Drop coredump infrastructure

Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/xb/xb.ml        |  6 +++---
 tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
 tools/ocaml/xenstored/stdext.ml  |  2 +-
 tools/ocaml/xenstored/utils.ml   | 18 +++++++++---------
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 50944b5fd6..aa2cf98223 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -84,7 +84,7 @@ let read_mmap back con s len =
 
 let read con s len =
 	match con.backend with
-	| Fd backfd     -> read_fd backfd con s len
+	| Fd backfd     -> read_fd backfd con (Bytes.of_string s) len
 	| Xenmmap backmmap -> read_mmap backmmap con s len
 
 let write_fd back con s len =
@@ -98,7 +98,7 @@ let write_mmap back con s len =
 
 let write con s len =
 	match con.backend with
-	| Fd backfd     -> write_fd backfd con s len
+	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
 	| Xenmmap backmmap -> write_mmap backmmap con s len
 
 (* NB: can throw Reconnect *)
@@ -147,7 +147,7 @@ let input con =
 	| NoHdr (i, buf)      ->
 		(* we complete the partial header *)
 		if sz > 0 then
-			String.blit s 0 buf (Partial.header_size () - i) sz;
+			String.blit s 0 (Bytes.of_string buf) (Partial.header_size () - i) sz;
 		con.partial_in <- if sz = i then
 			HaveHdr (Partial.of_string buf) else NoHdr (i - sz, buf)
 	);
diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 0c0d03d0c4..d24abf8a3a 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -60,11 +60,11 @@ type logger =
 let truncate_line nb_chars line = 
 	if String.length line > nb_chars - 1 then
 		let len = max (nb_chars - 1) 2 in
-		let dst_line = String.create len in
-		String.blit line 0 dst_line 0 (len - 2);
-		dst_line.[len-2] <- '.'; 
-		dst_line.[len-1] <- '.';
-		dst_line
+		let dst_line = Bytes.create len in
+		Bytes.blit_string line 0 dst_line 0 (len - 2);
+		Bytes.set dst_line (len-2) '.'; 
+		Bytes.set dst_line (len-1) '.';
+		Bytes.to_string dst_line
 	else line
 
 let log_rotate ref_ch log_file log_nb_files =
@@ -252,13 +252,13 @@ let string_of_access_type = function
 	*)
 
 let sanitize_data data =
-	let data = String.copy data in
-	for i = 0 to String.length data - 1
+	let data = Bytes.copy data in
+	for i = 0 to Bytes.length data - 1
 	do
-		if data.[i] = '\000' then
-			data.[i] <- ' '
+		if Bytes.get data i = '\000' then
+			Bytes.set data i ' '
 	done;
-	String.escaped data
+	String.escaped (Bytes.to_string data)
 
 let activate_access_log = ref true
 let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -291,7 +291,7 @@ let access_logging ~con ~tid ?(data="") ~level access_type =
 				let date = string_of_date() in
 				let tid = string_of_tid ~con tid in
 				let access_type = string_of_access_type access_type in
-				let data = sanitize_data data in
+				let data = sanitize_data (Bytes.of_string data) in
 				let prefix = prefix !access_log_destination date in
 				let msg = Printf.sprintf "%s %s %s %s" prefix tid access_type data in
 				logger.write ~level msg)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index b8a8fd00e1..d05155c97e 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
 		let pid = Unix.getpid () in
 		let buf = string_of_int pid ^ "\n" in
 		let len = String.length buf in
-		if Unix.write fd buf 0 len <> len 
+		if Unix.write fd (Bytes.of_string buf) 0 len <> len 
 		then failwith "pidfile_write failed";
 	)
 	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index e89c1aff04..c96def7bee 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -45,23 +45,23 @@ let get_hierarchy path =
 
 let hexify s =
 	let hexseq_of_char c = sprintf "%02x" (Char.code c) in
-	let hs = String.create (String.length s * 2) in
+	let hs = Bytes.create (String.length s * 2) in
 	for i = 0 to String.length s - 1
 	do
 		let seq = hexseq_of_char s.[i] in
-		hs.[i * 2] <- seq.[0];
-		hs.[i * 2 + 1] <- seq.[1];
+		Bytes.set hs (i * 2) seq.[0];
+		Bytes.set hs (i * 2 + 1) seq.[1];
 	done;
-	hs
+	Bytes.to_string hs
 
 let unhexify hs =
 	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
-	let s = String.create (String.length hs / 2) in
-	for i = 0 to String.length s - 1
+	let s = Bytes.create (String.length hs / 2) in
+	for i = 0 to Bytes.length s - 1
 	do
-		s.[i] <- char_of_hexseq hs.[i * 2] hs.[i * 2 + 1]
+		Bytes.set s i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
 	done;
-	s
+	Bytes.to_string s
 
 let trim_path path =
 	try
@@ -85,7 +85,7 @@ let create_unix_socket name =
 let read_file_single_integer filename =
 	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
 	let buf = String.make 20 (char_of_int 0) in
-	let sz = Unix.read fd buf 0 20 in
+	let sz = Unix.read fd (Bytes.of_string buf) 0 20 in
 	Unix.close fd;
 	int_of_string (String.sub buf 0 sz)
 
-- 
2.14.3


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Tue, Feb 06, 2018 at 09:56:14PM +0000, Michael Young wrote:
> On Tue, 6 Feb 2018, Wei Liu wrote:
> 
> > On Tue, Jan 30, 2018 at 10:55:47PM +0000, Michael Young wrote:
> > > Xen built with ocaml 4.06 gives errors such as
> > > Error: This expression has type bytes but an expression was
> > > 	expected of type string
> > > as Byte and safe-strings which were introduced in 4.02 are the
> > > default in 4.06.
> > > This patch which is partly by Richard W.M. Jones of Red Hat
> > > from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
> > > fixes these issues.
> > > 
> > > Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> > > Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
> > 
> > Strangely this doesn't apply to staging. And after examining the
> > downloaded patch I'm not sure if my mail client is acting up. Do you
> > have a git branch that I can pull from?
> 
> The patch needed to be reduced as one of the sections being patched was
> removed by a recent patch. I am attaching the revised patch as a file in
> case there was also an email formatting issue.
> 

Thanks, I will try this today.

Wei.

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1576 bytes --]

On Tue, 2018-01-30 at 22:55 +0000, Michael Young wrote:
> Xen built with ocaml 4.06 gives errors such as
> Error: This expression has type bytes but an expression was
>  	expected of type string
> as Byte and safe-strings which were introduced in 4.02 are the
> default in 4.06.
> This patch which is partly by Richard W.M. Jones of Red Hat
> from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
> fixes these issues.
> 
> Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
>
So, with this patch, oxenstord does not start for me.

Systemd says this (sorry, it's not the full output.. I don't have it
right now, but can produce it):

systemctl status xenstored
 ...
 Active: failed (Result: protocol) since Thu 2018-02-08 17:47:56 CET; 12min ago
 ...
 Feb 08 17:47:56 Zhaman systemd[1]: xenstored.service: Failed with result 'protocol'.

Just running oxenstored from command line seems to exits with 0, but
there's not an oxenstored process running.

Getting rid of what is now commit "make xen ocaml safe-strings
compliant" (df1e4c6e7f8892e950433ff33c215df0cd7b30f7), things work
again for me.

Regards,
Dario

PS. There has been a v2 of this patch, I think, but I don't have in my
emails any longer, so I'm replying to this one.

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Thu, Feb 08, 2018 at 06:49:58PM +0100, Dario Faggioli wrote:
> On Tue, 2018-01-30 at 22:55 +0000, Michael Young wrote:
> > Xen built with ocaml 4.06 gives errors such as
> > Error: This expression has type bytes but an expression was
> >  	expected of type string
> > as Byte and safe-strings which were introduced in 4.02 are the
> > default in 4.06.
> > This patch which is partly by Richard W.M. Jones of Red Hat
> > from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
> > fixes these issues.
> > 
> > Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> > Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
> >
> So, with this patch, oxenstord does not start for me.
> 
> Systemd says this (sorry, it's not the full output.. I don't have it
> right now, but can produce it):
> 
> systemctl status xenstored
>  ...
>  Active: failed (Result: protocol) since Thu 2018-02-08 17:47:56 CET; 12min ago
>  ...
>  Feb 08 17:47:56 Zhaman systemd[1]: xenstored.service: Failed with result 'protocol'.
> 
> Just running oxenstored from command line seems to exits with 0, but
> there's not an oxenstored process running.
> 
> Getting rid of what is now commit "make xen ocaml safe-strings
> compliant" (df1e4c6e7f8892e950433ff33c215df0cd7b30f7), things work
> again for me.
> 

OK. I will revert the relevant commits in staging. I'm sure this will
block osstest flights.

Wei.

> Regards,
> Dario
> 
> PS. There has been a v2 of this patch, I think, but I don't have in my
> emails any longer, so I'm replying to this one.
> 
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Software Engineer @ SUSE https://www.suse.com/



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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Thu, Feb 08, 2018 at 06:03:48PM +0000, Wei Liu wrote:
> On Thu, Feb 08, 2018 at 06:49:58PM +0100, Dario Faggioli wrote:
> > On Tue, 2018-01-30 at 22:55 +0000, Michael Young wrote:
> > > Xen built with ocaml 4.06 gives errors such as
> > > Error: This expression has type bytes but an expression was
> > >  	expected of type string
> > > as Byte and safe-strings which were introduced in 4.02 are the
> > > default in 4.06.
> > > This patch which is partly by Richard W.M. Jones of Red Hat
> > > from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
> > > fixes these issues.
> > > 
> > > Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> > > Reviewed-by: Christian Lindig <christian.lindig@citrix.com>
> > >
> > So, with this patch, oxenstord does not start for me.
> > 
> > Systemd says this (sorry, it's not the full output.. I don't have it
> > right now, but can produce it):
> > 
> > systemctl status xenstored
> >  ...
> >  Active: failed (Result: protocol) since Thu 2018-02-08 17:47:56 CET; 12min ago
> >  ...
> >  Feb 08 17:47:56 Zhaman systemd[1]: xenstored.service: Failed with result 'protocol'.
> > 
> > Just running oxenstored from command line seems to exits with 0, but
> > there's not an oxenstored process running.
> > 
> > Getting rid of what is now commit "make xen ocaml safe-strings
> > compliant" (df1e4c6e7f8892e950433ff33c215df0cd7b30f7), things work
> > again for me.
> > 
> 
> OK. I will revert the relevant commits in staging. I'm sure this will
> block osstest flights.
> 

Correction: osstest currently still runs Debian jessie, which has ocaml
4.01, which means the bump to 4.02 in staging is likely cause oxenstored
to be disabled. New flights could still pass but that's due to
oxenstored not getting tested. It is convoluted, I know. :-/

I need to at least revert the safe-string patches in staging. As for the
version bump, I'm not so sure. On one hand it is useless in its own and
leaving the bump in tree actually stops the testing of oxenstored, on
the other hand it is a must-have for safe-string patch, assuming we will
have a proper safe-string fix soon-ish we can leave them in staging.

Christian, do you have any idea when you can look into fixing the
safe-string patch?

And osstest should really be upgraded to strech (which has 4.02).

Wei.


> Wei.
> 
> > Regards,
> > Dario
> > 
> > PS. There has been a v2 of this patch, I think, but I don't have in my
> > emails any longer, so I'm replying to this one.
> > 
> > -- 
> > <<This happens because I choose it to happen!>> (Raistlin Majere)
> > -----------------------------------------------------------------
> > Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> > Software Engineer @ SUSE https://www.suse.com/
> 
> 

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Christian Lindig]

> On 8. Feb 2018, at 18:24, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> Christian, do you have any idea when you can look into fixing the
> safe-string patch?

Sorry, I can’t make a promise because of my other obligations. I do wonder, though: this patch did not come out of nowhere but supposedly was working - what is different here?

In any case, I will reproduce the problem and take a look.

The patch was doing the right thing for the future but there is no harm not having it right away. The entire XenServer toolstack is facing the same problem of moving to immutable strings and so far we have done it where it was easy and moved to use upstream libraries that use immutable string. In our own code, this is still a massive task.

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]

On Fri, 2018-02-09 at 09:20 +0000, Christian Lindig wrote:
> > On 8. Feb 2018, at 18:24, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > Christian, do you have any idea when you can look into fixing the
> > safe-string patch?
> 
> Sorry, I can’t make a promise because of my other obligations. I do
> wonder, though: this patch did not come out of nowhere but supposedly
> was working - what is different here?
> 
Well, the only thing that I know about OCAML is that it is, for me as
an Italian, a word a little bit difficult to pronounce. :-D

But I'm happy to try to give some more details on what fails and how,
if you direct me. :-)

The testbox where this happens is a Debian unstable which has ocaml
4.05.0-10.

> In any case, I will reproduce the problem and take a look.
> 
Andrew said on IRC that he's seen something similar happening on some
of yours XenRT runs... but really, I'm happy to provide more info.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Fri, Feb 09, 2018 at 09:20:33AM +0000, Christian Lindig wrote:
> 
> 
> > On 8. Feb 2018, at 18:24, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > Christian, do you have any idea when you can look into fixing the
> > safe-string patch?
> 
> Sorry, I can’t make a promise because of my other obligations. I do wonder, though: this patch did not come out of nowhere but supposedly was working - what is different here?
> 

No worries. I have reverted some patches in xen.git to get things going
again.

Wei.

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On Fri, 9 Feb 2018, Christian Lindig wrote:

> Sorry, I can’t make a promise because of my other obligations. I do 
> wonder, though: this patch did not come out of nowhere but supposedly 
> was working - what is different here?

The patch was from Fedora and is broken there too! It fixes the build 
issues but it doesn't look like anyone tested it (I use xenstored).

I have been trying to narrow down where the problem is and I think there 
are actually two issues;
one in tools/ocaml/xenstored/utils.ml which I think causes an error like 
Fatal error: exception Failure("int_of_string")
and one in tools/ocaml/libs/xb/xb.ml which I think causes an error like
Fatal error: exception Failure("evtchn bind_interdomain failed")

 	Michael Young

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Mon, 12 Feb 2018, Wei Liu wrote:

> On Fri, Feb 09, 2018 at 09:20:33AM +0000, Christian Lindig wrote:
>>
>>
>>> On 8. Feb 2018, at 18:24, Wei Liu <wei.liu2@citrix.com> wrote:
>>>
>>> Christian, do you have any idea when you can look into fixing the
>>> safe-string patch?
>>
>> Sorry, I can’t make a promise because of my other obligations. I do wonder, though: this patch did not come out of nowhere but supposedly was working - what is different here?
>>
>
> No worries. I have reverted some patches in xen.git to get things going
> again.

I have had a go at fixing the patch and my revised attempt is attached. 
I suspect it could be tidied up, but it works for me.

 	Michael Young

[-- Attachment #2: Type: text/plain, Size: 8719 bytes --]

From 550ffe177842e3fd9f38c78e07072fa7c7b591a5 Mon Sep 17 00:00:00 2001
From: Michael Young <m.a.young@durham.ac.uk>
Date: Fri, 9 Mar 2018 22:31:41 +0000
Subject: [PATCH v3] make xen ocaml safe-strings compliant

Xen built with ocaml 4.06 gives errors such as
Error: This expression has type bytes but an expression was
        expected of type string
as Byte and safe-strings which were introduced in 4.02 are the
default in 4.06.
This patch which is partly by Richard W.M. Jones of Red Hat
from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
fixes these issues.

v3: rework patches for xb.ml and /utils.ml to fix broken code relating
to Unix.read.  Update xb.mli to match changes in xb.ml.

Signed-off-by: Michael Young <m.a.young@durham.ac.uk>

---
 tools/ocaml/libs/xb/xb.ml        | 18 ++++++++++--------
 tools/ocaml/libs/xb/xb.mli       | 10 +++++-----
 tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
 tools/ocaml/xenstored/stdext.ml  |  2 +-
 tools/ocaml/xenstored/utils.ml   | 20 ++++++++++----------
 5 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 50944b5fd6..42ae8d2bd8 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -40,7 +40,7 @@ type backend_fd =
 
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
 
-type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * string
+type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
 
 type t =
 {
@@ -52,7 +52,7 @@ type t =
 }
 
 let init_partial_in () = NoHdr
-	(Partial.header_size (), String.make (Partial.header_size()) '\000')
+	(Partial.header_size (), Bytes.make (Partial.header_size()) '\000')
 
 let reconnect t = match t.backend with
 	| Fd _ ->
@@ -76,7 +76,9 @@ let read_fd back con s len =
 	rd
 
 let read_mmap back con s len =
-	let rd = Xs_ring.read back.mmap s len in
+	let stmp = String.make len (char_of_int 0) in
+	let rd = Xs_ring.read back.mmap stmp len in
+	Bytes.blit_string stmp 0 s 0 rd;
 	back.work_again <- (rd > 0);
 	if rd > 0 then
 		back.eventchn_notify ();
@@ -98,7 +100,7 @@ let write_mmap back con s len =
 
 let write con s len =
 	match con.backend with
-	| Fd backfd     -> write_fd backfd con s len
+	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
 	| Xenmmap backmmap -> write_mmap backmmap con s len
 
 (* NB: can throw Reconnect *)
@@ -129,7 +131,7 @@ let input con =
 		| NoHdr   (i, buf)    -> i in
 
 	(* try to get more data from input stream *)
-	let s = String.make to_read '\000' in
+	let s = Bytes.make to_read '\000' in
 	let sz = if to_read > 0 then read con s to_read else 0 in
 
 	(
@@ -137,7 +139,7 @@ let input con =
 	| HaveHdr partial_pkt ->
 		(* we complete the data *)
 		if sz > 0 then
-			Partial.append partial_pkt s sz;
+			Partial.append partial_pkt (Bytes.to_string s) sz;
 		if Partial.to_complete partial_pkt = 0 then (
 			let pkt = Packet.of_partialpkt partial_pkt in
 			con.partial_in <- init_partial_in ();
@@ -147,9 +149,9 @@ let input con =
 	| NoHdr (i, buf)      ->
 		(* we complete the partial header *)
 		if sz > 0 then
-			String.blit s 0 buf (Partial.header_size () - i) sz;
+			Bytes.blit s 0 buf (Partial.header_size () - i) sz;
 		con.partial_in <- if sz = i then
-			HaveHdr (Partial.of_string buf) else NoHdr (i - sz, buf)
+			HaveHdr (Partial.of_string (Bytes.to_string buf)) else NoHdr (i - sz, buf)
 	);
 	!newpacket
 
diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
index b4d705201f..d566011fc7 100644
--- a/tools/ocaml/libs/xb/xb.mli
+++ b/tools/ocaml/libs/xb/xb.mli
@@ -65,7 +65,7 @@ type backend_mmap = {
 }
 type backend_fd = { fd : Unix.file_descr; }
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
-type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * string
+type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
 type t = {
   backend : backend;
   pkt_in : Packet.t Queue.t;
@@ -76,10 +76,10 @@ type t = {
 val init_partial_in : unit -> partial_buf
 val reconnect : t -> unit
 val queue : t -> Packet.t -> unit
-val read_fd : backend_fd -> 'a -> string -> int -> int
-val read_mmap : backend_mmap -> 'a -> string -> int -> int
-val read : t -> string -> int -> int
-val write_fd : backend_fd -> 'a -> string -> int -> int
+val read_fd : backend_fd -> 'a -> bytes -> int -> int
+val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
+val read : t -> bytes -> int -> int
+val write_fd : backend_fd -> 'a -> bytes -> int -> int
 val write_mmap : backend_mmap -> 'a -> string -> int -> int
 val write : t -> string -> int -> int
 val output : t -> bool
diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 0c0d03d0c4..d24abf8a3a 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -60,11 +60,11 @@ type logger =
 let truncate_line nb_chars line = 
 	if String.length line > nb_chars - 1 then
 		let len = max (nb_chars - 1) 2 in
-		let dst_line = String.create len in
-		String.blit line 0 dst_line 0 (len - 2);
-		dst_line.[len-2] <- '.'; 
-		dst_line.[len-1] <- '.';
-		dst_line
+		let dst_line = Bytes.create len in
+		Bytes.blit_string line 0 dst_line 0 (len - 2);
+		Bytes.set dst_line (len-2) '.'; 
+		Bytes.set dst_line (len-1) '.';
+		Bytes.to_string dst_line
 	else line
 
 let log_rotate ref_ch log_file log_nb_files =
@@ -252,13 +252,13 @@ let string_of_access_type = function
 	*)
 
 let sanitize_data data =
-	let data = String.copy data in
-	for i = 0 to String.length data - 1
+	let data = Bytes.copy data in
+	for i = 0 to Bytes.length data - 1
 	do
-		if data.[i] = '\000' then
-			data.[i] <- ' '
+		if Bytes.get data i = '\000' then
+			Bytes.set data i ' '
 	done;
-	String.escaped data
+	String.escaped (Bytes.to_string data)
 
 let activate_access_log = ref true
 let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -291,7 +291,7 @@ let access_logging ~con ~tid ?(data="") ~level access_type =
 				let date = string_of_date() in
 				let tid = string_of_tid ~con tid in
 				let access_type = string_of_access_type access_type in
-				let data = sanitize_data data in
+				let data = sanitize_data (Bytes.of_string data) in
 				let prefix = prefix !access_log_destination date in
 				let msg = Printf.sprintf "%s %s %s %s" prefix tid access_type data in
 				logger.write ~level msg)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index b8a8fd00e1..d05155c97e 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
 		let pid = Unix.getpid () in
 		let buf = string_of_int pid ^ "\n" in
 		let len = String.length buf in
-		if Unix.write fd buf 0 len <> len 
+		if Unix.write fd (Bytes.of_string buf) 0 len <> len 
 		then failwith "pidfile_write failed";
 	)
 	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index e89c1aff04..4fc542dd51 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -45,23 +45,23 @@ let get_hierarchy path =
 
 let hexify s =
 	let hexseq_of_char c = sprintf "%02x" (Char.code c) in
-	let hs = String.create (String.length s * 2) in
+	let hs = Bytes.create (String.length s * 2) in
 	for i = 0 to String.length s - 1
 	do
 		let seq = hexseq_of_char s.[i] in
-		hs.[i * 2] <- seq.[0];
-		hs.[i * 2 + 1] <- seq.[1];
+		Bytes.set hs (i * 2) seq.[0];
+		Bytes.set hs (i * 2 + 1) seq.[1];
 	done;
-	hs
+	Bytes.to_string hs
 
 let unhexify hs =
 	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
-	let s = String.create (String.length hs / 2) in
-	for i = 0 to String.length s - 1
+	let s = Bytes.create (String.length hs / 2) in
+	for i = 0 to Bytes.length s - 1
 	do
-		s.[i] <- char_of_hexseq hs.[i * 2] hs.[i * 2 + 1]
+		Bytes.set s i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
 	done;
-	s
+	Bytes.to_string s
 
 let trim_path path =
 	try
@@ -84,10 +84,10 @@ let create_unix_socket name =
 
 let read_file_single_integer filename =
 	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
-	let buf = String.make 20 (char_of_int 0) in
+	let buf = Bytes.make 20 (char_of_int 0) in
 	let sz = Unix.read fd buf 0 20 in
 	Unix.close fd;
-	int_of_string (String.sub buf 0 sz)
+	int_of_string (Bytes.to_string (Bytes.sub buf 0 sz))
 
 let path_complete path connection_path =
 	if String.get path 0 <> '/' then
-- 
2.14.3


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Christian Lindig]

> On 9. Mar 2018, at 22:57, Michael Young <m.a.young@durham.ac.uk> wrote:
> 
> I have had a go at fixing the patch and my revised attempt is attached. I suspect it could be tidied up, but it works for me.

Thank you for giving this another go. What is the key difference to the previous patch which compiled but lead to lock up at run time? I briefly tried your patch and can confirm that it compiles and looks clean except for two trailing spaces. 

— Christian

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

On Fri, 9 Mar 2018, Christian Lindig wrote:

>
>
>> On 9. Mar 2018, at 22:57, Michael Young <m.a.young@durham.ac.uk> wrote:
>>
>> I have had a go at fixing the patch and my revised attempt is attached. I suspect it could be tidied up, but it works for me.
>
> Thank you for giving this another go. What is the key difference to the previous patch which compiled but lead to lock up at run time? I briefly tried your patch and can confirm that it compiles and looks clean except for two trailing spaces.
>
> — Christian
>
> Acked-by: Christian Lindig <christian.lindig@citrix.com>

The problem with the old patch is illustrated by the following section 
from the old patch for tools/ocaml/xenstored/utils.ml
@@ -85,7 +85,7 @@ let create_unix_socket name =
  let read_file_single_integer filename =
         let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
         let buf = String.make 20 (char_of_int 0) in
-       let sz = Unix.read fd buf 0 20 in
+       let sz = Unix.read fd (Bytes.of_string buf) 0 20 in
         Unix.close fd;
         int_of_string (String.sub buf 0 sz)

where the patch makes Unix.read write to a Bytes copy of buf and buf 
itself is unchanged, so int_of_string sees a string of null characters 
rather than a string to convert into a number. The net result is that 
information being read by oxenstored from the hypervisor is corrupted or 
lost.
The same basic problem also occurred in a couple of places in 
the old patch of tools/ocaml/libs/xb/xb.ml.
My fix for this is to switch to Bytes at an earlier stage, so for example 
the corresponding section in the new patch becomes
@@ -84,10 +84,10 @@ let create_unix_socket name =

  let read_file_single_integer filename =
  	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
-	let buf = String.make 20 (char_of_int 0) in
+	let buf = Bytes.make 20 (char_of_int 0) in
  	let sz = Unix.read fd buf 0 20 in
  	Unix.close fd;
-	int_of_string (String.sub buf 0 sz)
+	int_of_string (Bytes.to_string (Bytes.sub buf 0 sz))

  let path_complete path connection_path =
  	if String.get path 0 <> '/' then

 	Michael Young

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Christian Lindig]

> The problem with the old patch is illustrated by the following section 
> from the old patch for tools/ocaml/xenstored/utils.ml
> @@ -85,7 +85,7 @@ let create_unix_socket name =
>  let read_file_single_integer filename =
>         let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
>         let buf = String.make 20 (char_of_int 0) in
> -       let sz = Unix.read fd buf 0 20 in
> +       let sz = Unix.read fd (Bytes.of_string buf) 0 20 in
>         Unix.close fd;
>         int_of_string (String.sub buf 0 sz)
>
> where the patch makes Unix.read write to a Bytes copy of buf and buf 
> itself is unchanged, so int_of_string sees a string of null characters 
> rather than a string to convert into a number.

Good analysis. (Bytes.of_string buf) created a buffer for the result 
from read() for which we have no handle.

Reviewing the new patch I believe it is sound. The (new) signature of 
read_mmap is

> val read_mmap : backend_mmap -> 'a -> bytes -> int -> int

The new implementation is below - argument s used to be a string value 
and is now a bytes value. I would suggest to reflect this in the names 
(using b instead of s) as this is about conversion between strings and 
bytes.
>   let read_mmap back con s len =
> -	let rd = Xs_ring.read back.mmap s len in
> +	let stmp = String.make len (char_of_int 0) in
> +	let rd = Xs_ring.read back.mmap stmp len in
> +	Bytes.blit_string stmp 0 s 0 rd;
>   	back.work_again <- (rd > 0);
>   	if rd > 0 then
>   		back.eventchn_notify ();

Below are the functions that changed their type and where this also 
should be considered:
> -val read_fd : backend_fd -> 'a -> string -> int -> int
> -val read_mmap : backend_mmap -> 'a -> string -> int -> int
> -val read : t -> string -> int -> int
> -val write_fd : backend_fd -> 'a -> string -> int -> int
> +val read_fd : backend_fd -> 'a -> bytes -> int -> int
> +val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
> +val read : t -> bytes -> int -> int
> +val write_fd : backend_fd -> 'a -> bytes -> int -> int

-- Christian


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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Michael Young]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

On Mon, 12 Mar 2018, Christian Lindig wrote:

>>  The problem with the old patch is illustrated by the following section
>>  from the old patch for tools/ocaml/xenstored/utils.ml
>>  @@ -85,7 +85,7 @@ let create_unix_socket name =
>>   let read_file_single_integer filename =
>>          let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
>>          let buf = String.make 20 (char_of_int 0) in
>>  -       let sz = Unix.read fd buf 0 20 in
>>  +       let sz = Unix.read fd (Bytes.of_string buf) 0 20 in
>>          Unix.close fd;
>>          int_of_string (String.sub buf 0 sz)
>>
>>  where the patch makes Unix.read write to a Bytes copy of buf and buf
>>  itself is unchanged, so int_of_string sees a string of null characters
>>  rather than a string to convert into a number.
>
> Good analysis. (Bytes.of_string buf) created a buffer for the result from 
> read() for which we have no handle.
>
> Reviewing the new patch I believe it is sound. The (new) signature of 
> read_mmap is
>
>>  val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
>
> The new implementation is below - argument s used to be a string value and is 
> now a bytes value. I would suggest to reflect this in the names (using b 
> instead of s) as this is about conversion between strings and bytes.
>>    let read_mmap back con s len =
>>  -	let rd = Xs_ring.read back.mmap s len in
>>  +	let stmp = String.make len (char_of_int 0) in
>>  +	let rd = Xs_ring.read back.mmap stmp len in
>>  +	Bytes.blit_string stmp 0 s 0 rd;
>>     back.work_again <- (rd > 0);
>>     if rd > 0 then
>>      back.eventchn_notify ();
>
> Below are the functions that changed their type and where this also should be 
> considered:
>>  -val read_fd : backend_fd -> 'a -> string -> int -> int
>>  -val read_mmap : backend_mmap -> 'a -> string -> int -> int
>>  -val read : t -> string -> int -> int
>>  -val write_fd : backend_fd -> 'a -> string -> int -> int
>>  +val read_fd : backend_fd -> 'a -> bytes -> int -> int
>>  +val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
>>  +val read : t -> bytes -> int -> int
>>  +val write_fd : backend_fd -> 'a -> bytes -> int -> int
>
> -- Christian

Here is version 4 of the patch where I have replaced the uses of s with b 
where the patch changes it from string to bytes. I have also removed the 
two trailing spaces and changed stmp back to s.

 	Michael Young

[-- Attachment #2: Type: text/plain, Size: 9432 bytes --]

From da088e4eef2bbea4be262e12db4c36960ff5145a Mon Sep 17 00:00:00 2001
From: Michael Young <m.a.young@durham.ac.uk>
Date: Mon, 12 Mar 2018 18:49:29 +0000
Subject: [PATCH v4] make xen ocaml safe-strings compliant

Xen built with ocaml 4.06 gives errors such as
Error: This expression has type bytes but an expression was
        expected of type string
as Byte and safe-strings which were introduced in 4.02 are the
default in 4.06.
This patch which is partly by Richard W.M. Jones of Red Hat
from https://bugzilla.redhat.com/show_bug.cgi?id=1526703
fixes these issues.

v4: Where string s is now bytes, rename it to b.

Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
---
 tools/ocaml/libs/xb/xb.ml        | 34 ++++++++++++++++++----------------
 tools/ocaml/libs/xb/xb.mli       | 10 +++++-----
 tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
 tools/ocaml/xenstored/stdext.ml  |  2 +-
 tools/ocaml/xenstored/utils.ml   | 20 ++++++++++----------
 5 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 50944b5fd6..519842723b 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -40,7 +40,7 @@ type backend_fd =
 
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
 
-type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * string
+type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
 
 type t =
 {
@@ -52,7 +52,7 @@ type t =
 }
 
 let init_partial_in () = NoHdr
-	(Partial.header_size (), String.make (Partial.header_size()) '\000')
+	(Partial.header_size (), Bytes.make (Partial.header_size()) '\000')
 
 let reconnect t = match t.backend with
 	| Fd _ ->
@@ -69,26 +69,28 @@ let reconnect t = match t.backend with
 
 let queue con pkt = Queue.push pkt con.pkt_out
 
-let read_fd back con s len =
-	let rd = Unix.read back.fd s 0 len in
+let read_fd back con b len =
+	let rd = Unix.read back.fd b 0 len in
 	if rd = 0 then
 		raise End_of_file;
 	rd
 
-let read_mmap back con s len =
+let read_mmap back con b len =
+	let s = String.make len (char_of_int 0) in
 	let rd = Xs_ring.read back.mmap s len in
+	Bytes.blit_string s 0 b 0 rd;
 	back.work_again <- (rd > 0);
 	if rd > 0 then
 		back.eventchn_notify ();
 	rd
 
-let read con s len =
+let read con b len =
 	match con.backend with
-	| Fd backfd     -> read_fd backfd con s len
-	| Xenmmap backmmap -> read_mmap backmmap con s len
+	| Fd backfd     -> read_fd backfd con b len
+	| Xenmmap backmmap -> read_mmap backmmap con b len
 
-let write_fd back con s len =
-	Unix.write back.fd s 0 len
+let write_fd back con b len =
+	Unix.write back.fd b 0 len
 
 let write_mmap back con s len =
 	let ws = Xs_ring.write back.mmap s len in
@@ -98,7 +100,7 @@ let write_mmap back con s len =
 
 let write con s len =
 	match con.backend with
-	| Fd backfd     -> write_fd backfd con s len
+	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
 	| Xenmmap backmmap -> write_mmap backmmap con s len
 
 (* NB: can throw Reconnect *)
@@ -129,15 +131,15 @@ let input con =
 		| NoHdr   (i, buf)    -> i in
 
 	(* try to get more data from input stream *)
-	let s = String.make to_read '\000' in
-	let sz = if to_read > 0 then read con s to_read else 0 in
+	let b = Bytes.make to_read '\000' in
+	let sz = if to_read > 0 then read con b to_read else 0 in
 
 	(
 	match con.partial_in with
 	| HaveHdr partial_pkt ->
 		(* we complete the data *)
 		if sz > 0 then
-			Partial.append partial_pkt s sz;
+			Partial.append partial_pkt (Bytes.to_string b) sz;
 		if Partial.to_complete partial_pkt = 0 then (
 			let pkt = Packet.of_partialpkt partial_pkt in
 			con.partial_in <- init_partial_in ();
@@ -147,9 +149,9 @@ let input con =
 	| NoHdr (i, buf)      ->
 		(* we complete the partial header *)
 		if sz > 0 then
-			String.blit s 0 buf (Partial.header_size () - i) sz;
+			Bytes.blit b 0 buf (Partial.header_size () - i) sz;
 		con.partial_in <- if sz = i then
-			HaveHdr (Partial.of_string buf) else NoHdr (i - sz, buf)
+			HaveHdr (Partial.of_string (Bytes.to_string buf)) else NoHdr (i - sz, buf)
 	);
 	!newpacket
 
diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
index b4d705201f..d566011fc7 100644
--- a/tools/ocaml/libs/xb/xb.mli
+++ b/tools/ocaml/libs/xb/xb.mli
@@ -65,7 +65,7 @@ type backend_mmap = {
 }
 type backend_fd = { fd : Unix.file_descr; }
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
-type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * string
+type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
 type t = {
   backend : backend;
   pkt_in : Packet.t Queue.t;
@@ -76,10 +76,10 @@ type t = {
 val init_partial_in : unit -> partial_buf
 val reconnect : t -> unit
 val queue : t -> Packet.t -> unit
-val read_fd : backend_fd -> 'a -> string -> int -> int
-val read_mmap : backend_mmap -> 'a -> string -> int -> int
-val read : t -> string -> int -> int
-val write_fd : backend_fd -> 'a -> string -> int -> int
+val read_fd : backend_fd -> 'a -> bytes -> int -> int
+val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
+val read : t -> bytes -> int -> int
+val write_fd : backend_fd -> 'a -> bytes -> int -> int
 val write_mmap : backend_mmap -> 'a -> string -> int -> int
 val write : t -> string -> int -> int
 val output : t -> bool
diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 0c0d03d0c4..e3c769fb2c 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -60,11 +60,11 @@ type logger =
 let truncate_line nb_chars line = 
 	if String.length line > nb_chars - 1 then
 		let len = max (nb_chars - 1) 2 in
-		let dst_line = String.create len in
-		String.blit line 0 dst_line 0 (len - 2);
-		dst_line.[len-2] <- '.'; 
-		dst_line.[len-1] <- '.';
-		dst_line
+		let dst_line = Bytes.create len in
+		Bytes.blit_string line 0 dst_line 0 (len - 2);
+		Bytes.set dst_line (len-2) '.';
+		Bytes.set dst_line (len-1) '.';
+		Bytes.to_string dst_line
 	else line
 
 let log_rotate ref_ch log_file log_nb_files =
@@ -252,13 +252,13 @@ let string_of_access_type = function
 	*)
 
 let sanitize_data data =
-	let data = String.copy data in
-	for i = 0 to String.length data - 1
+	let data = Bytes.copy data in
+	for i = 0 to Bytes.length data - 1
 	do
-		if data.[i] = '\000' then
-			data.[i] <- ' '
+		if Bytes.get data i = '\000' then
+			Bytes.set data i ' '
 	done;
-	String.escaped data
+	String.escaped (Bytes.to_string data)
 
 let activate_access_log = ref true
 let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -291,7 +291,7 @@ let access_logging ~con ~tid ?(data="") ~level access_type =
 				let date = string_of_date() in
 				let tid = string_of_tid ~con tid in
 				let access_type = string_of_access_type access_type in
-				let data = sanitize_data data in
+				let data = sanitize_data (Bytes.of_string data) in
 				let prefix = prefix !access_log_destination date in
 				let msg = Printf.sprintf "%s %s %s %s" prefix tid access_type data in
 				logger.write ~level msg)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index b8a8fd00e1..41411ee535 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
 		let pid = Unix.getpid () in
 		let buf = string_of_int pid ^ "\n" in
 		let len = String.length buf in
-		if Unix.write fd buf 0 len <> len 
+		if Unix.write fd (Bytes.of_string buf) 0 len <> len
 		then failwith "pidfile_write failed";
 	)
 	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index e89c1aff04..5fcb042351 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -45,23 +45,23 @@ let get_hierarchy path =
 
 let hexify s =
 	let hexseq_of_char c = sprintf "%02x" (Char.code c) in
-	let hs = String.create (String.length s * 2) in
+	let hs = Bytes.create (String.length s * 2) in
 	for i = 0 to String.length s - 1
 	do
 		let seq = hexseq_of_char s.[i] in
-		hs.[i * 2] <- seq.[0];
-		hs.[i * 2 + 1] <- seq.[1];
+		Bytes.set hs (i * 2) seq.[0];
+		Bytes.set hs (i * 2 + 1) seq.[1];
 	done;
-	hs
+	Bytes.to_string hs
 
 let unhexify hs =
 	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
-	let s = String.create (String.length hs / 2) in
-	for i = 0 to String.length s - 1
+	let b = Bytes.create (String.length hs / 2) in
+	for i = 0 to Bytes.length b - 1
 	do
-		s.[i] <- char_of_hexseq hs.[i * 2] hs.[i * 2 + 1]
+		Bytes.set b i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
 	done;
-	s
+	Bytes.to_string b
 
 let trim_path path =
 	try
@@ -84,10 +84,10 @@ let create_unix_socket name =
 
 let read_file_single_integer filename =
 	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
-	let buf = String.make 20 (char_of_int 0) in
+	let buf = Bytes.make 20 (char_of_int 0) in
 	let sz = Unix.read fd buf 0 20 in
 	Unix.close fd;
-	int_of_string (String.sub buf 0 sz)
+	int_of_string (Bytes.to_string (Bytes.sub buf 0 sz))
 
 let path_complete path connection_path =
 	if String.get path 0 <> '/' then
-- 
2.14.3


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Christian Lindig]

On 12/03/18 19:35, Michael Young wrote:

> Here is version 4 of the patch where I have replaced the uses of s with b
> where the patch changes it from string to bytes. I have also removed the
> two trailing spaces and changed stmp back to s.
>      Michael Young
> 0001-make-xen-ocaml-safe-strings-compliant.patch
>
>  From da088e4eef2bbea4be262e12db4c36960ff5145a Mon Sep 17 00:00:00 2001
> From: Michael Young<m.a.young@durham.ac.uk>
> Date: Mon, 12 Mar 2018 18:49:29 +0000
> Subject: [PATCH v4] make xen ocaml safe-strings compliant
>
> Xen built with ocaml 4.06 gives errors such as
> Error: This expression has type bytes but an expression was
>          expected of type string
> as Byte and safe-strings which were introduced in 4.02 are the
> default in 4.06.
> This patch which is partly by Richard W.M. Jones of Red Hat
> fromhttps://bugzilla.redhat.com/show_bug.cgi?id=1526703
> fixes these issues.
>
> v4: Where string s is now bytes, rename it to b.
>
> Signed-off-by: Michael Young<m.a.young@durham.ac.uk>
> ---
>   tools/ocaml/libs/xb/xb.ml        | 34 ++++++++++++++++++----------------
>   tools/ocaml/libs/xb/xb.mli       | 10 +++++-----
>   tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
>   tools/ocaml/xenstored/stdext.ml  |  2 +-
>   tools/ocaml/xenstored/utils.ml   | 20 ++++++++++----------
>   5 files changed, 45 insertions(+), 43 deletions(-)

Reviewed-by: Christian Lindig<christian.lindig@citrix.com>

This patch is good to go. One caveat, though: the conversion between bytes and strings
is not just a cast but involves copying and thus has a performance cost which we
want to keep in mind. The OCaml low-level libraries like Unix.read return (mutable) bytes,
but most code expects (immutable) strings. I believe that it would be difficult to avoid
this in the long run and this patch is the right way to go.

-- Christian
  


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

Re: [PATCH 1/2] make xen ocaml safe-strings compliant

[Wei Liu]

On Tue, Mar 13, 2018 at 09:29:49AM +0000, Christian Lindig wrote:
> On 12/03/18 19:35, Michael Young wrote:
> 
> > Here is version 4 of the patch where I have replaced the uses of s with b
> > where the patch changes it from string to bytes. I have also removed the
> > two trailing spaces and changed stmp back to s.
> >      Michael Young
> > 0001-make-xen-ocaml-safe-strings-compliant.patch
> > 
> >  From da088e4eef2bbea4be262e12db4c36960ff5145a Mon Sep 17 00:00:00 2001
> > From: Michael Young<m.a.young@durham.ac.uk>
> > Date: Mon, 12 Mar 2018 18:49:29 +0000
> > Subject: [PATCH v4] make xen ocaml safe-strings compliant
> > 
> > Xen built with ocaml 4.06 gives errors such as
> > Error: This expression has type bytes but an expression was
> >          expected of type string
> > as Byte and safe-strings which were introduced in 4.02 are the
> > default in 4.06.
> > This patch which is partly by Richard W.M. Jones of Red Hat
> > fromhttps://bugzilla.redhat.com/show_bug.cgi?id=1526703
> > fixes these issues.
> > 
> > v4: Where string s is now bytes, rename it to b.
> > 
> > Signed-off-by: Michael Young<m.a.young@durham.ac.uk>
> > ---
> >   tools/ocaml/libs/xb/xb.ml        | 34 ++++++++++++++++++----------------
> >   tools/ocaml/libs/xb/xb.mli       | 10 +++++-----
> >   tools/ocaml/xenstored/logging.ml | 22 +++++++++++-----------
> >   tools/ocaml/xenstored/stdext.ml  |  2 +-
> >   tools/ocaml/xenstored/utils.ml   | 20 ++++++++++----------
> >   5 files changed, 45 insertions(+), 43 deletions(-)
> 
> Reviewed-by: Christian Lindig<christian.lindig@citrix.com>

Thanks. I will apply this patch soon.

Wei.

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