xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Wei Liu <wei.liu2@citrix.com>
To: Jonathan Davies <jonathan.davies@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jon Ludlam <jonathan.ludlam@citrix.com>,
	Euan Harris <euan.harris@citrix.com>,
	Dave Scott <dave@recoil.org>,
	xen-devel@lists.xenproject.org,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH 3/7] oxenstored: refactor request processing
Date: Tue, 29 Mar 2016 17:38:08 +0100	[thread overview]
Message-ID: <20160329163808.GA1024@citrix.com> (raw)
In-Reply-To: <20160329090829.GE3316@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

  parent reply	other threads:[~2016-03-29 16:38 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-17 17:51 [PATCH 0/7] oxenstored: improve transaction conflict handling Jonathan Davies
2016-03-17 17:51 ` [PATCH 1/7] oxenstored: refactor putting response on wire Jonathan Davies
2016-03-17 17:51 ` [PATCH 2/7] oxenstored: remove some unused parameters Jonathan Davies
2016-03-17 17:51 ` [PATCH 3/7] oxenstored: refactor request processing Jonathan Davies
2016-03-24 22:22   ` Boris Ostrovsky
2016-03-24 22:49     ` Andrew Cooper
2016-03-24 23:57       ` Boris Ostrovsky
2016-03-29  9:08         ` Jonathan Davies
2016-03-29 12:45           ` Boris Ostrovsky
2016-03-29 16:38           ` Wei Liu [this message]
2016-03-29 19:41             ` David Scott
2016-03-30 15:46               ` Jonathan Davies
2016-03-30 15:53                 ` Wei Liu
2016-03-17 17:51 ` [PATCH 4/7] oxenstored: keep track of each transaction's operations Jonathan Davies
2016-03-17 17:51 ` [PATCH 5/7] oxenstored: move functions that process simple operations Jonathan Davies
2016-03-17 17:51 ` [PATCH 6/7] oxenstored: replay transaction upon conflict Jonathan Davies
2016-03-17 17:51 ` [PATCH 7/7] oxenstored: log request and response during transaction replay Jonathan Davies
2016-03-18 14:33 ` [PATCH 0/7] oxenstored: improve transaction conflict handling Konrad Rzeszutek Wilk
2016-03-18 16:21   ` Jonathan Davies
2016-03-18 16:36   ` Wei Liu
2016-03-19 11:30     ` David Scott

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160329163808.GA1024@citrix.com \
    --to=wei.liu2@citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=dave@recoil.org \
    --cc=euan.harris@citrix.com \
    --cc=jonathan.davies@citrix.com \
    --cc=jonathan.ludlam@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH 3/7] oxenstored: refactor request processing' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).