diff mbox

[3/7] oxenstored: refactor request processing

Message ID 1458237075-13777-4-git-send-email-jonathan.davies@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jonathan Davies March 17, 2016, 5:51 p.m. UTC
Encapsulate the request in a record that is passed from do_input to
process_packet and input_handle_error.

This will be helpful when keeping track of the requests made as part of a
transaction.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Boris Ostrovsky March 24, 2016, 10:22 p.m. UTC | #1
On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> Encapsulate the request in a record that is passed from do_input to
> process_packet and input_handle_error.
>
> This will be helpful when keeping track of the requests made as part of a
> transaction.
>
> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> Reviewed-by: Euan Harris <euan.harris@citrix.com>
> ---
>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
> index 7a73669..c92bec7 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -344,11 +344,11 @@ let function_of_type ty =
>   	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>   	| _                              -> reply_ack do_error
>   
> -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>   	let reply_error e =
>   		Packet.Error e in
>   	try
> -		fct con t doms cons data
> +		fct con t doms cons req.Packet.data
>   	with
>   	| Define.Invalid_path          -> reply_error "EINVAL"
>   	| Define.Already_exist         -> reply_error "EEXIST"
> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>   (**
>    * Nothrow guarantee.
>    *)
> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> +let process_packet ~store ~cons ~doms ~con ~req =
> +	let ty = req.Packet.ty in
> +	let tid = req.Packet.tid in
> +	let rid = req.Packet.rid in
>   	try
>   		let fct = function_of_type ty in
>   		let t =
> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>   			else
>   				Connection.get_transaction con tid
>   			in
> -		let response = input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data in
> +		let response = input_handle_error ~cons ~doms ~fct ~con ~t ~req in
>   
>   		(* Put the response on the wire *)
>   		send_response ty con t rid response
> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>   	if newpacket then (
>   		let packet = Connection.pop_in con in
>   		let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> +		let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> +

I think this change breaks the build with older ocaml versions:

root@haswell> ocamlopt -v
The OCaml native-code compiler, version 4.00.1
Standard library directory: /usr/lib64/ocaml
root@haswell> ocamlopt -g -ccopt "    " -dtypes -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F 
-warn-error F -c -o process.cmx process.ml
root@haswell>


root@ovs104> ocamlopt -v
The Objective Caml native-code compiler, version 3.11.2
Standard library directory: /usr/lib64/ocaml
root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F 
-warn-error F -c -o process.cmx process.ml
File "process.ml", line 487, characters 23-24:
Error: Syntax error
root@ovs104>

I don't know much about ocaml (OK, I know *nothing* about ocaml) so I 
can't say what exactly might be wrong.

-boris
Andrew Cooper March 24, 2016, 10:49 p.m. UTC | #2
On 24/03/2016 22:22, Boris Ostrovsky wrote:
> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>> Encapsulate the request in a record that is passed from do_input to
>> process_packet and input_handle_error.
>>
>> This will be helpful when keeping track of the requests made as part
>> of a
>> transaction.
>>
>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>> ---
>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/ocaml/xenstored/process.ml
>> b/tools/ocaml/xenstored/process.ml
>> index 7a73669..c92bec7 100644
>> --- a/tools/ocaml/xenstored/process.ml
>> +++ b/tools/ocaml/xenstored/process.ml
>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>       | _                              -> reply_ack do_error
>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>       let reply_error e =
>>           Packet.Error e in
>>       try
>> -        fct con t doms cons data
>> +        fct con t doms cons req.Packet.data
>>       with
>>       | Define.Invalid_path          -> reply_error "EINVAL"
>>       | Define.Already_exist         -> reply_error "EEXIST"
>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>> ~t ~rid ~data =
>>   (**
>>    * Nothrow guarantee.
>>    *)
>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>> +let process_packet ~store ~cons ~doms ~con ~req =
>> +    let ty = req.Packet.ty in
>> +    let tid = req.Packet.tid in
>> +    let rid = req.Packet.rid in
>>       try
>>           let fct = function_of_type ty in
>>           let t =
>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>> ~rid ~ty ~data =
>>               else
>>                   Connection.get_transaction con tid
>>               in
>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>> ~t ~rid ~data in
>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>> ~req in
>>             (* Put the response on the wire *)
>>           send_response ty con t rid response
>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>       if newpacket then (
>>           let packet = Connection.pop_in con in
>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>> +
>
> I think this change breaks the build with older ocaml versions:
>
> root@haswell> ocamlopt -v
> The OCaml native-code compiler, version 4.00.1
> Standard library directory: /usr/lib64/ocaml
> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> -warn-error F -c -o process.cmx process.ml
> root@haswell>
>
>
> root@ovs104> ocamlopt -v
> The Objective Caml native-code compiler, version 3.11.2
> Standard library directory: /usr/lib64/ocaml
> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> -warn-error F -c -o process.cmx process.ml
> File "process.ml", line 487, characters 23-24:
> Error: Syntax error
> root@ovs104>
>
> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> can't say what exactly might be wrong.

Could you perhaps try this instead?

let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
Packet.data} in

It is most likely that the older version of Ocaml can't infer the order
of fields.

~Andrew
Boris Ostrovsky March 24, 2016, 11:57 p.m. UTC | #3
On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>> Encapsulate the request in a record that is passed from do_input to
>>> process_packet and input_handle_error.
>>>
>>> This will be helpful when keeping track of the requests made as part
>>> of a
>>> transaction.
>>>
>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>> ---
>>>    tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/ocaml/xenstored/process.ml
>>> b/tools/ocaml/xenstored/process.ml
>>> index 7a73669..c92bec7 100644
>>> --- a/tools/ocaml/xenstored/process.ml
>>> +++ b/tools/ocaml/xenstored/process.ml
>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>        | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>        | _                              -> reply_ack do_error
>>>    -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>        let reply_error e =
>>>            Packet.Error e in
>>>        try
>>> -        fct con t doms cons data
>>> +        fct con t doms cons req.Packet.data
>>>        with
>>>        | Define.Invalid_path          -> reply_error "EINVAL"
>>>        | Define.Already_exist         -> reply_error "EEXIST"
>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>> ~t ~rid ~data =
>>>    (**
>>>     * Nothrow guarantee.
>>>     *)
>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>> +    let ty = req.Packet.ty in
>>> +    let tid = req.Packet.tid in
>>> +    let rid = req.Packet.rid in
>>>        try
>>>            let fct = function_of_type ty in
>>>            let t =
>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>> ~rid ~ty ~data =
>>>                else
>>>                    Connection.get_transaction con tid
>>>                in
>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>> ~t ~rid ~data in
>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>> ~req in
>>>              (* Put the response on the wire *)
>>>            send_response ty con t rid response
>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>        if newpacket then (
>>>            let packet = Connection.pop_in con in
>>>            let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>> +
>> I think this change breaks the build with older ocaml versions:
>>
>> root@haswell> ocamlopt -v
>> The OCaml native-code compiler, version 4.00.1
>> Standard library directory: /usr/lib64/ocaml
>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>> -warn-error F -c -o process.cmx process.ml
>> root@haswell>
>>
>>
>> root@ovs104> ocamlopt -v
>> The Objective Caml native-code compiler, version 3.11.2
>> Standard library directory: /usr/lib64/ocaml
>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>> -warn-error F -c -o process.cmx process.ml
>> File "process.ml", line 487, characters 23-24:
>> Error: Syntax error
>> root@ovs104>
>>
>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>> can't say what exactly might be wrong.
> Could you perhaps try this instead?
>
> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> Packet.data} in
>
> It is most likely that the older version of Ocaml can't infer the order
> of fields.
>
> ~Andrew


No, it now gives me "Error: Unbound record field label tid"

-boris
Jonathan Davies March 29, 2016, 9:08 a.m. UTC | #4
On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> >On 24/03/2016 22:22, Boris Ostrovsky wrote:
> >>On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> >>>Encapsulate the request in a record that is passed from do_input to
> >>>process_packet and input_handle_error.
> >>>
> >>>This will be helpful when keeping track of the requests made as part
> >>>of a
> >>>transaction.
> >>>
> >>>Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> >>>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> >>>Reviewed-by: Euan Harris <euan.harris@citrix.com>
> >>>---
> >>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> >>>   1 file changed, 10 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/tools/ocaml/xenstored/process.ml
> >>>b/tools/ocaml/xenstored/process.ml
> >>>index 7a73669..c92bec7 100644
> >>>--- a/tools/ocaml/xenstored/process.ml
> >>>+++ b/tools/ocaml/xenstored/process.ml
> >>>@@ -344,11 +344,11 @@ let function_of_type ty =
> >>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> >>>       | _                              -> reply_ack do_error
> >>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> >>>+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> >>>       let reply_error e =
> >>>           Packet.Error e in
> >>>       try
> >>>-        fct con t doms cons data
> >>>+        fct con t doms cons req.Packet.data
> >>>       with
> >>>       | Define.Invalid_path          -> reply_error "EINVAL"
> >>>       | Define.Already_exist         -> reply_error "EEXIST"
> >>>@@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>~t ~rid ~data =
> >>>   (**
> >>>    * Nothrow guarantee.
> >>>    *)
> >>>-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> >>>+let process_packet ~store ~cons ~doms ~con ~req =
> >>>+    let ty = req.Packet.ty in
> >>>+    let tid = req.Packet.tid in
> >>>+    let rid = req.Packet.rid in
> >>>       try
> >>>           let fct = function_of_type ty in
> >>>           let t =
> >>>@@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> >>>~rid ~ty ~data =
> >>>               else
> >>>                   Connection.get_transaction con tid
> >>>               in
> >>>-        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>~t ~rid ~data in
> >>>+        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> >>>~req in
> >>>             (* Put the response on the wire *)
> >>>           send_response ty con t rid response
> >>>@@ -412,11 +415,13 @@ let do_input store cons doms con =
> >>>       if newpacket then (
> >>>           let packet = Connection.pop_in con in
> >>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> >>>+        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> >>>+
> >>I think this change breaks the build with older ocaml versions:
> >>
> >>root@haswell> ocamlopt -v
> >>The OCaml native-code compiler, version 4.00.1
> >>Standard library directory: /usr/lib64/ocaml
> >>root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>-warn-error F -c -o process.cmx process.ml
> >>root@haswell>
> >>
> >>
> >>root@ovs104> ocamlopt -v
> >>The Objective Caml native-code compiler, version 3.11.2
> >>Standard library directory: /usr/lib64/ocaml
> >>root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>-warn-error F -c -o process.cmx process.ml
> >>File "process.ml", line 487, characters 23-24:
> >>Error: Syntax error
> >>root@ovs104>
> >>
> >>I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> >>can't say what exactly might be wrong.
> >Could you perhaps try this instead?
> >
> >let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> >Packet.data} in
> >
> >It is most likely that the older version of Ocaml can't infer the order
> >of fields.
> >
> >~Andrew
> 
> 
> No, it now gives me "Error: Unbound record field label tid"
> 
> -boris

Andrew's guess was close, but the wrong way around -- please could you try the
following with the older compiler?

let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in

I was using a syntactic feature of OCaml called 'field punning' which is
generally considered good practice and makes for more readable code. It looks
like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
consistent with Boris' findings.

What's the policy here -- is there a defined version of the OCaml compiler which
tools/ocaml needs to be able to compile with?

Thanks,
Jonathan
Boris Ostrovsky March 29, 2016, 12:45 p.m. UTC | #5
On 03/29/2016 05:08 AM, Jonathan Davies wrote:
> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>>>> Encapsulate the request in a record that is passed from do_input to
>>>>> process_packet and input_handle_error.
>>>>>
>>>>> This will be helpful when keeping track of the requests made as part
>>>>> of a
>>>>> transaction.
>>>>>
>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>>>> ---
>>>>>    tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/ocaml/xenstored/process.ml
>>>>> b/tools/ocaml/xenstored/process.ml
>>>>> index 7a73669..c92bec7 100644
>>>>> --- a/tools/ocaml/xenstored/process.ml
>>>>> +++ b/tools/ocaml/xenstored/process.ml
>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>>>        | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>>>        | _                              -> reply_ack do_error
>>>>>    -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>>>        let reply_error e =
>>>>>            Packet.Error e in
>>>>>        try
>>>>> -        fct con t doms cons data
>>>>> +        fct con t doms cons req.Packet.data
>>>>>        with
>>>>>        | Define.Invalid_path          -> reply_error "EINVAL"
>>>>>        | Define.Already_exist         -> reply_error "EEXIST"
>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>> ~t ~rid ~data =
>>>>>    (**
>>>>>     * Nothrow guarantee.
>>>>>     *)
>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>>>> +    let ty = req.Packet.ty in
>>>>> +    let tid = req.Packet.tid in
>>>>> +    let rid = req.Packet.rid in
>>>>>        try
>>>>>            let fct = function_of_type ty in
>>>>>            let t =
>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>>>> ~rid ~ty ~data =
>>>>>                else
>>>>>                    Connection.get_transaction con tid
>>>>>                in
>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>> ~t ~rid ~data in
>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>>>> ~req in
>>>>>              (* Put the response on the wire *)
>>>>>            send_response ty con t rid response
>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>>>        if newpacket then (
>>>>>            let packet = Connection.pop_in con in
>>>>>            let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>>>> +
>>>> I think this change breaks the build with older ocaml versions:
>>>>
>>>> root@haswell> ocamlopt -v
>>>> The OCaml native-code compiler, version 4.00.1
>>>> Standard library directory: /usr/lib64/ocaml
>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>> -warn-error F -c -o process.cmx process.ml
>>>> root@haswell>
>>>>
>>>>
>>>> root@ovs104> ocamlopt -v
>>>> The Objective Caml native-code compiler, version 3.11.2
>>>> Standard library directory: /usr/lib64/ocaml
>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>> -warn-error F -c -o process.cmx process.ml
>>>> File "process.ml", line 487, characters 23-24:
>>>> Error: Syntax error
>>>> root@ovs104>
>>>>
>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>>>> can't say what exactly might be wrong.
>>> Could you perhaps try this instead?
>>>
>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
>>> Packet.data} in
>>>
>>> It is most likely that the older version of Ocaml can't infer the order
>>> of fields.
>>>
>>> ~Andrew
>>
>> No, it now gives me "Error: Unbound record field label tid"
>>
>> -boris
> Andrew's guess was close, but the wrong way around -- please could you try the
> following with the older compiler?
>
> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
>
> I was using a syntactic feature of OCaml called 'field punning' which is
> generally considered good practice and makes for more readable code. It looks
> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> consistent with Boris' findings.

Yes, this fixes it, thanks.

> What's the policy here -- is there a defined version of the OCaml compiler which
> tools/ocaml needs to be able to compile with?

Not to answer this question directly but more as an FYI --- this version 
of ocaml is found on Oracle Linux 6.2, which AFAIK is actively supported.

-boris
Wei Liu March 29, 2016, 4:38 p.m. UTC | #6
On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> > On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> > >On 24/03/2016 22:22, Boris Ostrovsky wrote:
> > >>On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> > >>>Encapsulate the request in a record that is passed from do_input to
> > >>>process_packet and input_handle_error.
> > >>>
> > >>>This will be helpful when keeping track of the requests made as part
> > >>>of a
> > >>>transaction.
> > >>>
> > >>>Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> > >>>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > >>>Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> > >>>Reviewed-by: Euan Harris <euan.harris@citrix.com>
> > >>>---
> > >>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> > >>>   1 file changed, 10 insertions(+), 5 deletions(-)
> > >>>
> > >>>diff --git a/tools/ocaml/xenstored/process.ml
> > >>>b/tools/ocaml/xenstored/process.ml
> > >>>index 7a73669..c92bec7 100644
> > >>>--- a/tools/ocaml/xenstored/process.ml
> > >>>+++ b/tools/ocaml/xenstored/process.ml
> > >>>@@ -344,11 +344,11 @@ let function_of_type ty =
> > >>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> > >>>       | _                              -> reply_ack do_error
> > >>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> > >>>+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> > >>>       let reply_error e =
> > >>>           Packet.Error e in
> > >>>       try
> > >>>-        fct con t doms cons data
> > >>>+        fct con t doms cons req.Packet.data
> > >>>       with
> > >>>       | Define.Invalid_path          -> reply_error "EINVAL"
> > >>>       | Define.Already_exist         -> reply_error "EEXIST"
> > >>>@@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> > >>>~t ~rid ~data =
> > >>>   (**
> > >>>    * Nothrow guarantee.
> > >>>    *)
> > >>>-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> > >>>+let process_packet ~store ~cons ~doms ~con ~req =
> > >>>+    let ty = req.Packet.ty in
> > >>>+    let tid = req.Packet.tid in
> > >>>+    let rid = req.Packet.rid in
> > >>>       try
> > >>>           let fct = function_of_type ty in
> > >>>           let t =
> > >>>@@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> > >>>~rid ~ty ~data =
> > >>>               else
> > >>>                   Connection.get_transaction con tid
> > >>>               in
> > >>>-        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> > >>>~t ~rid ~data in
> > >>>+        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> > >>>~req in
> > >>>             (* Put the response on the wire *)
> > >>>           send_response ty con t rid response
> > >>>@@ -412,11 +415,13 @@ let do_input store cons doms con =
> > >>>       if newpacket then (
> > >>>           let packet = Connection.pop_in con in
> > >>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> > >>>+        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> > >>>+
> > >>I think this change breaks the build with older ocaml versions:
> > >>
> > >>root@haswell> ocamlopt -v
> > >>The OCaml native-code compiler, version 4.00.1
> > >>Standard library directory: /usr/lib64/ocaml
> > >>root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> > >>-warn-error F -c -o process.cmx process.ml
> > >>root@haswell>
> > >>
> > >>
> > >>root@ovs104> ocamlopt -v
> > >>The Objective Caml native-code compiler, version 3.11.2
> > >>Standard library directory: /usr/lib64/ocaml
> > >>root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> > >>-warn-error F -c -o process.cmx process.ml
> > >>File "process.ml", line 487, characters 23-24:
> > >>Error: Syntax error
> > >>root@ovs104>
> > >>
> > >>I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> > >>can't say what exactly might be wrong.
> > >Could you perhaps try this instead?
> > >
> > >let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> > >Packet.data} in
> > >
> > >It is most likely that the older version of Ocaml can't infer the order
> > >of fields.
> > >
> > >~Andrew
> > 
> > 
> > No, it now gives me "Error: Unbound record field label tid"
> > 
> > -boris
> 
> Andrew's guess was close, but the wrong way around -- please could you try the
> following with the older compiler?
> 
> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> 
> I was using a syntactic feature of OCaml called 'field punning' which is
> generally considered good practice and makes for more readable code. It looks
> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> consistent with Boris' findings.
> 
> What's the policy here -- is there a defined version of the OCaml compiler which
> tools/ocaml needs to be able to compile with?

It is not explicitly listed in README or INSTALL. The ocaml tools
maintainer (Dave in this case) is welcome to provide the minimum version
required.

Meanwhile, I don't think we should break existing build without pinning
down the minimum required version first, so we should fix Boris's
breakage.  The fix seems simple enough anyway.

Wei.

> 
> Thanks,
> Jonathan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
David Scott March 29, 2016, 7:41 p.m. UTC | #7
> On 29 Mar 2016, at 17:38, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
>> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
>>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
>>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>>>>> Encapsulate the request in a record that is passed from do_input to
>>>>>> process_packet and input_handle_error.
>>>>>> 
>>>>>> This will be helpful when keeping track of the requests made as part
>>>>>> of a
>>>>>> transaction.
>>>>>> 
>>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>>>>> ---
>>>>>>  tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>> 
>>>>>> diff --git a/tools/ocaml/xenstored/process.ml
>>>>>> b/tools/ocaml/xenstored/process.ml
>>>>>> index 7a73669..c92bec7 100644
>>>>>> --- a/tools/ocaml/xenstored/process.ml
>>>>>> +++ b/tools/ocaml/xenstored/process.ml
>>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>>>>      | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>>>>      | _                              -> reply_ack do_error
>>>>>>  -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>>>>      let reply_error e =
>>>>>>          Packet.Error e in
>>>>>>      try
>>>>>> -        fct con t doms cons data
>>>>>> +        fct con t doms cons req.Packet.data
>>>>>>      with
>>>>>>      | Define.Invalid_path          -> reply_error "EINVAL"
>>>>>>      | Define.Already_exist         -> reply_error "EEXIST"
>>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>>> ~t ~rid ~data =
>>>>>>  (**
>>>>>>   * Nothrow guarantee.
>>>>>>   *)
>>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>>>>> +    let ty = req.Packet.ty in
>>>>>> +    let tid = req.Packet.tid in
>>>>>> +    let rid = req.Packet.rid in
>>>>>>      try
>>>>>>          let fct = function_of_type ty in
>>>>>>          let t =
>>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>>>>> ~rid ~ty ~data =
>>>>>>              else
>>>>>>                  Connection.get_transaction con tid
>>>>>>              in
>>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>>> ~t ~rid ~data in
>>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>>>>> ~req in
>>>>>>            (* Put the response on the wire *)
>>>>>>          send_response ty con t rid response
>>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>>>>      if newpacket then (
>>>>>>          let packet = Connection.pop_in con in
>>>>>>          let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>>>>> +
>>>>> I think this change breaks the build with older ocaml versions:
>>>>> 
>>>>> root@haswell> ocamlopt -v
>>>>> The OCaml native-code compiler, version 4.00.1
>>>>> Standard library directory: /usr/lib64/ocaml
>>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>>> -warn-error F -c -o process.cmx process.ml
>>>>> root@haswell>
>>>>> 
>>>>> 
>>>>> root@ovs104> ocamlopt -v
>>>>> The Objective Caml native-code compiler, version 3.11.2
>>>>> Standard library directory: /usr/lib64/ocaml
>>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>>> -warn-error F -c -o process.cmx process.ml
>>>>> File "process.ml", line 487, characters 23-24:
>>>>> Error: Syntax error
>>>>> root@ovs104>
>>>>> 
>>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>>>>> can't say what exactly might be wrong.
>>>> Could you perhaps try this instead?
>>>> 
>>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
>>>> Packet.data} in
>>>> 
>>>> It is most likely that the older version of Ocaml can't infer the order
>>>> of fields.
>>>> 
>>>> ~Andrew
>>> 
>>> 
>>> No, it now gives me "Error: Unbound record field label tid"
>>> 
>>> -boris
>> 
>> Andrew's guess was close, but the wrong way around -- please could you try the
>> following with the older compiler?
>> 
>> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
>> 
>> I was using a syntactic feature of OCaml called 'field punning' which is
>> generally considered good practice and makes for more readable code. It looks
>> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
>> consistent with Boris' findings.
>> 
>> What's the policy here -- is there a defined version of the OCaml compiler which
>> tools/ocaml needs to be able to compile with?
> 
> It is not explicitly listed in README or INSTALL. The ocaml tools
> maintainer (Dave in this case) is welcome to provide the minimum version
> required.
> 
> Meanwhile, I don't think we should break existing build without pinning
> down the minimum required version first, so we should fix Boris's
> breakage.  The fix seems simple enough anyway.

It looks like the fix is small and easy — I think this is good for now.

Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.

Cheers,
Dave

> 
> Wei.
> 
>> 
>> Thanks,
>> Jonathan
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
Jonathan Davies March 30, 2016, 3:46 p.m. UTC | #8
On Tue, Mar 29, 2016 at 08:41:54PM +0100, David Scott wrote:
> 
> > On 29 Mar 2016, at 17:38, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
> >> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> >>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> >>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
> >>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> >>>>>> Encapsulate the request in a record that is passed from do_input to
> >>>>>> process_packet and input_handle_error.
> >>>>>> 
> >>>>>> This will be helpful when keeping track of the requests made as part
> >>>>>> of a
> >>>>>> transaction.
> >>>>>> 
> >>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> >>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> >>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
> >>>>>> ---
> >>>>>>  tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> >>>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/tools/ocaml/xenstored/process.ml
> >>>>>> b/tools/ocaml/xenstored/process.ml
> >>>>>> index 7a73669..c92bec7 100644
> >>>>>> --- a/tools/ocaml/xenstored/process.ml
> >>>>>> +++ b/tools/ocaml/xenstored/process.ml
> >>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
> >>>>>>      | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> >>>>>>      | _                              -> reply_ack do_error
> >>>>>>  -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> >>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> >>>>>>      let reply_error e =
> >>>>>>          Packet.Error e in
> >>>>>>      try
> >>>>>> -        fct con t doms cons data
> >>>>>> +        fct con t doms cons req.Packet.data
> >>>>>>      with
> >>>>>>      | Define.Invalid_path          -> reply_error "EINVAL"
> >>>>>>      | Define.Already_exist         -> reply_error "EEXIST"
> >>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>>>> ~t ~rid ~data =
> >>>>>>  (**
> >>>>>>   * Nothrow guarantee.
> >>>>>>   *)
> >>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> >>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
> >>>>>> +    let ty = req.Packet.ty in
> >>>>>> +    let tid = req.Packet.tid in
> >>>>>> +    let rid = req.Packet.rid in
> >>>>>>      try
> >>>>>>          let fct = function_of_type ty in
> >>>>>>          let t =
> >>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> >>>>>> ~rid ~ty ~data =
> >>>>>>              else
> >>>>>>                  Connection.get_transaction con tid
> >>>>>>              in
> >>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>>>> ~t ~rid ~data in
> >>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> >>>>>> ~req in
> >>>>>>            (* Put the response on the wire *)
> >>>>>>          send_response ty con t rid response
> >>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
> >>>>>>      if newpacket then (
> >>>>>>          let packet = Connection.pop_in con in
> >>>>>>          let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> >>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> >>>>>> +
> >>>>> I think this change breaks the build with older ocaml versions:
> >>>>> 
> >>>>> root@haswell> ocamlopt -v
> >>>>> The OCaml native-code compiler, version 4.00.1
> >>>>> Standard library directory: /usr/lib64/ocaml
> >>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>>>> -warn-error F -c -o process.cmx process.ml
> >>>>> root@haswell>
> >>>>> 
> >>>>> 
> >>>>> root@ovs104> ocamlopt -v
> >>>>> The Objective Caml native-code compiler, version 3.11.2
> >>>>> Standard library directory: /usr/lib64/ocaml
> >>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>>>> -warn-error F -c -o process.cmx process.ml
> >>>>> File "process.ml", line 487, characters 23-24:
> >>>>> Error: Syntax error
> >>>>> root@ovs104>
> >>>>> 
> >>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> >>>>> can't say what exactly might be wrong.
> >>>> Could you perhaps try this instead?
> >>>> 
> >>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> >>>> Packet.data} in
> >>>> 
> >>>> It is most likely that the older version of Ocaml can't infer the order
> >>>> of fields.
> >>>> 
> >>>> ~Andrew
> >>> 
> >>> 
> >>> No, it now gives me "Error: Unbound record field label tid"
> >>> 
> >>> -boris
> >> 
> >> Andrew's guess was close, but the wrong way around -- please could you try the
> >> following with the older compiler?
> >> 
> >> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> >> 
> >> I was using a syntactic feature of OCaml called 'field punning' which is
> >> generally considered good practice and makes for more readable code. It looks
> >> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> >> consistent with Boris' findings.
> >> 
> >> What's the policy here -- is there a defined version of the OCaml compiler which
> >> tools/ocaml needs to be able to compile with?
> > 
> > It is not explicitly listed in README or INSTALL. The ocaml tools
> > maintainer (Dave in this case) is welcome to provide the minimum version
> > required.
> > 
> > Meanwhile, I don't think we should break existing build without pinning
> > down the minimum required version first, so we should fix Boris's
> > breakage.  The fix seems simple enough anyway.
> 
> It looks like the fix is small and easy — I think this is good for now.
> 
> Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.

OK; sounds sensible.

Since the original patch has already been committed to staging, I presume you'd
like me to formally submit a standalone patch that fixes this issue. I will post
it separately.

If I'm wrong and you'd like me to post a v2 of the whole series, let me know.

Thanks,
Jonathan
Wei Liu March 30, 2016, 3:53 p.m. UTC | #9
On Wed, Mar 30, 2016 at 04:46:58PM +0100, Jonathan Davies wrote:
[...]
> > >> Andrew's guess was close, but the wrong way around -- please could you try the
> > >> following with the older compiler?
> > >> 
> > >> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> > >> 
> > >> I was using a syntactic feature of OCaml called 'field punning' which is
> > >> generally considered good practice and makes for more readable code. It looks
> > >> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> > >> consistent with Boris' findings.
> > >> 
> > >> What's the policy here -- is there a defined version of the OCaml compiler which
> > >> tools/ocaml needs to be able to compile with?
> > > 
> > > It is not explicitly listed in README or INSTALL. The ocaml tools
> > > maintainer (Dave in this case) is welcome to provide the minimum version
> > > required.
> > > 
> > > Meanwhile, I don't think we should break existing build without pinning
> > > down the minimum required version first, so we should fix Boris's
> > > breakage.  The fix seems simple enough anyway.
> > 
> > It looks like the fix is small and easy — I think this is good for now.
> > 
> > Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.
> 
> OK; sounds sensible.
> 
> Since the original patch has already been committed to staging, I presume you'd
> like me to formally submit a standalone patch that fixes this issue. I will post
> it separately.
> 

Yes, a standalone patch to fix the issue is fine.

Wei.

> If I'm wrong and you'd like me to post a v2 of the whole series, let me know.
> 
> Thanks,
> Jonathan
diff mbox

Patch

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 7a73669..c92bec7 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -344,11 +344,11 @@  let function_of_type ty =
 	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
 	| _                              -> reply_ack do_error
 
-let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
 	let reply_error e =
 		Packet.Error e in
 	try
-		fct con t doms cons data
+		fct con t doms cons req.Packet.data
 	with
 	| Define.Invalid_path          -> reply_error "EINVAL"
 	| Define.Already_exist         -> reply_error "EEXIST"
@@ -370,7 +370,10 @@  let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
 (**
  * Nothrow guarantee.
  *)
-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
+let process_packet ~store ~cons ~doms ~con ~req =
+	let ty = req.Packet.ty in
+	let tid = req.Packet.tid in
+	let rid = req.Packet.rid in
 	try
 		let fct = function_of_type ty in
 		let t =
@@ -379,7 +382,7 @@  let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
 			else
 				Connection.get_transaction con tid
 			in
-		let response = input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data in
+		let response = input_handle_error ~cons ~doms ~fct ~con ~t ~req in
 
 		(* Put the response on the wire *)
 		send_response ty con t rid response
@@ -412,11 +415,13 @@  let do_input store cons doms con =
 	if newpacket then (
 		let packet = Connection.pop_in con in
 		let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
+		let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
+
 		(* As we don't log IO, do not call an unnecessary sanitize_data 
 		   info "[%s] -> [%d] %s \"%s\""
 		         (Connection.get_domstr con) tid
 		         (Xenbus.Xb.Op.to_string ty) (sanitize_data data); *)
-		process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data;
+		process_packet ~store ~cons ~doms ~con ~req;
 		write_access_log ~ty ~tid ~con ~data;
 		Connection.incr_ops con;
 	)