[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/7] oxenstored: refactor request processing
On Tue, Mar 29, 2016 at 08:41:54PM +0100, David Scott wrote: > > > On 29 Mar 2016, at 17:38, Wei Liu <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxx> > >>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > >>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@xxxxxxxxxx> > >>>>>> Reviewed-by: Euan Harris <euan.harris@xxxxxxxxxx> > >>>>>> --- > >>>>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |