Xen devel - Sep 2013 - [PATCH] Fix ocaml build in 4.1; problem with 4.1.6 release

(CC to xen-devel added)

Ian Jackson writes ("Problem with 4.1.6
release"):
> Jan Beulich writes ("please sign-and-tag 4.2.3 and 4.1.6"):
> > Ian - could you also create tarballs as usual?
> 
> The 4.1.6 tarball failed its build test.  This is due to an actual
> build failure in 4.1.6.  See below.  This is probably a result of
> 070ab4c505934951f86f42dd8403cf62bc5822f0 "oxenstored: Protect
> oxenstored from malicious domains".
Now confirmed by a local rebuild with 070ab4 reverted.
Possible fix below.  This builds but I haven''t tested it.
> It appears that when I backported this change I this change from 4.2.x
> to 4.1.x, I cannot have done a build test :-/.  (I have upgraded my
> workstation in between but the error doesn''t seem likely to be due
to
> a compiler version change.)
> 
> The backport to 4.2.x involved conflicts, which I fixed up, and did do
> a build test for so it looks like I resolved the conflict correctly.
> The 4.1.x backport (of the 4.2.x patch) didn''t involve textual
> conflicts but it seems to have a semantic conflict.
> 
> The osstest testing system doesn''t install ocaml compilers,
because
> that makes it use oxenstored by default and the last time I attempted
> to do this it produced regressions.  So the push gate didn''t catch
> this problem.
>    
> I have already made and pushed a signed tag for 4.1.6.  That suggests
> that we should abandon the 4.1.6 version number, in favour of 4.1.6.1
> maybe.
> 
> We also have to decide what to do with the code.  We shouldn''t
really
> simply revert this fix, which is security-relevant.
> 
> CC the authors of the patch which when backported became 070ab4c50593,
> and Andrew Cooper who knows something about this code.
> 
> Ian.
> 
> make[5]: Entering directory
`/u/iwj/work/xen.git/tools/ocaml/xenstored''
>  MLI      symbol.cmi
>  MLI      trie.cmi
>  MLOPT    define.cmx
>  MLOPT    stdext.cmx
>  MLOPT    trie.cmx
>  MLOPT    config.cmx
>  MLOPT    logging.cmx
> File "logging.ml", line 113, characters 3-4:
> Warning 11: this match case is unused.
>  MLOPT    quota.cmx
>  MLOPT    perms.cmx
>  MLOPT    symbol.cmx
>  MLOPT    utils.cmx
>  MLOPT    store.cmx
>  MLOPT    disk.cmx
>  MLOPT    transaction.cmx
>  MLOPT    event.cmx
>  MLOPT    domain.cmx
>  MLOPT    domains.cmx
>  MLOPT    connection.cmx
>  MLOPT    connections.cmx
>  MLOPT    parse_arg.cmx
>  MLOPT    process.cmx
> File "process.ml", line 375, characters 3-8:
> Error: Unbound value error
commit 7e792ffe54adc2d9fcc210baa8140f210a841c31
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Mon Sep 9 15:30:42 2013 +0100

    oxenstored: Fix process.ml build after 070ab4c50593
    
    This change:
      070ab4c505934951f86f42dd8403cf62bc5822f0
      "oxenstored: Protect oxenstored from malicious domains"
    broke the build because it had an unresolved semantic (but not
    textual) conflict with
      c69fddbd5dfa3004aaf2d0f2dde00c9ec3dd6d5d
      "tools/ocaml: Remove log library from tools/ocaml/libs"
    (which is in 4.2 but not 4.1)
    
    Fix this by using the 4.1.x idiom in the new error handling introduced
    in 070ab4c50593.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index bd87646..5c81755 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -372,8 +372,8 @@ let do_input store cons doms con  		try
 			Connection.do_input con
 		with Failure exp ->
-			error "caught exception %s" exp;
-			error "got a bad client %s" (sprintf "%-8s"
(Connection.get_domstr con));
+			Logs.error "general" "caught exception %s" exp;
+			Logs.error "general" "got a bad client %s" (sprintf
"%-8s" (Connection.get_domstr con));
 			Connection.mark_as_bad con;
 			false
 	in

Ian Jackson writes ("Problem with 4.1.6
release"):
> The 4.1.6 tarball failed its build test.  This is due to an actual
> build failure in 4.1.6.  See below.  This is probably a result of
> 070ab4c505934951f86f42dd8403cf62bc5822f0 "oxenstored: Protect
> oxenstored from malicious domains".
Ian Jackson writes ("[PATCH] Fix ocaml build in 4.1; problem with 4.1.6
release"):
> Possible fix below.  This builds but I haven''t tested it.
I think we should proceed as follows:
 * Ditch the version number 4.1.6 in favour of 4.1.7
 * Get some review and acks on my fix patch
 * Tag qemu-xen-traditional with 4.1.7 and update Xen 4.1''s Config.mk
 * Push the fix patch into 4.1-testing
 * Edit the release notes for 4.1.6 into ones for 4.1.7, and mention
   there that 4.1.6 was not actually released due to a problem during
   the release process.
 * Release the result, 4.1.7, perhaps without waiting for the push gate

Ian.

>>> On 09.09.13 at 16:59, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> I think we should proceed as follows:
>  * Ditch the version number 4.1.6 in favour of 4.1.7
>  * Get some review and acks on my fix patch
>  * Tag qemu-xen-traditional with 4.1.7 and update Xen 4.1''s
Config.mk
>  * Push the fix patch into 4.1-testing
>  * Edit the release notes for 4.1.6 into ones for 4.1.7, and mention
>    there that 4.1.6 was not actually released due to a problem during
>    the release process.
>  * Release the result, 4.1.7, perhaps without waiting for the push gate
All fine with me with the possible exception of using 4.1.7 -
personally I think that 4.1.6.1 would be more appropriate.

Jan

On 09/09/13 15:35, Ian Jackson wrote:
> (CC to xen-devel added)
>
> Ian Jackson writes ("Problem with 4.1.6 release"):
>> Jan Beulich writes ("please sign-and-tag 4.2.3 and 4.1.6"):
>>> Ian - could you also create tarballs as usual?
>>
>> The 4.1.6 tarball failed its build test.  This is due to an actual
>> build failure in 4.1.6.  See below.  This is probably a result of
>> 070ab4c505934951f86f42dd8403cf62bc5822f0 "oxenstored: Protect
>> oxenstored from malicious domains".
>
> Now confirmed by a local rebuild with 070ab4 reverted.
> Possible fix below.  This builds but I haven''t tested it.
>
>> It appears that when I backported this change I this change from 4.2.x
>> to 4.1.x, I cannot have done a build test :-/.  (I have upgraded my
>> workstation in between but the error doesn''t seem likely to be
due to
>> a compiler version change.)
>>
>> The backport to 4.2.x involved conflicts, which I fixed up, and did do
>> a build test for so it looks like I resolved the conflict correctly.
>> The 4.1.x backport (of the 4.2.x patch) didn''t involve textual
>> conflicts but it seems to have a semantic conflict.
>>
>> The osstest testing system doesn''t install ocaml compilers,
because
>> that makes it use oxenstored by default and the last time I attempted
>> to do this it produced regressions.  So the push gate didn''t
catch
>> this problem.
>>
>> I have already made and pushed a signed tag for 4.1.6.  That suggests
>> that we should abandon the 4.1.6 version number, in favour of 4.1.6.1
>> maybe.
>>
>> We also have to decide what to do with the code.  We shouldn''t
really
>> simply revert this fix, which is security-relevant.
>>
>> CC the authors of the patch which when backported became 070ab4c50593,
>> and Andrew Cooper who knows something about this code.
>>
>> Ian.
>>
>> make[5]: Entering directory
`/u/iwj/work/xen.git/tools/ocaml/xenstored''
>>   MLI      symbol.cmi
>>   MLI      trie.cmi
>>   MLOPT    define.cmx
>>   MLOPT    stdext.cmx
>>   MLOPT    trie.cmx
>>   MLOPT    config.cmx
>>   MLOPT    logging.cmx
>> File "logging.ml", line 113, characters 3-4:
>> Warning 11: this match case is unused.
>>   MLOPT    quota.cmx
>>   MLOPT    perms.cmx
>>   MLOPT    symbol.cmx
>>   MLOPT    utils.cmx
>>   MLOPT    store.cmx
>>   MLOPT    disk.cmx
>>   MLOPT    transaction.cmx
>>   MLOPT    event.cmx
>>   MLOPT    domain.cmx
>>   MLOPT    domains.cmx
>>   MLOPT    connection.cmx
>>   MLOPT    connections.cmx
>>   MLOPT    parse_arg.cmx
>>   MLOPT    process.cmx
>> File "process.ml", line 375, characters 3-8:
>> Error: Unbound value error
>
> commit 7e792ffe54adc2d9fcc210baa8140f210a841c31
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Mon Sep 9 15:30:42 2013 +0100
>
>      oxenstored: Fix process.ml build after 070ab4c50593
>
>      This change:
>        070ab4c505934951f86f42dd8403cf62bc5822f0
>        "oxenstored: Protect oxenstored from malicious domains"
>      broke the build because it had an unresolved semantic (but not
>      textual) conflict with
>        c69fddbd5dfa3004aaf2d0f2dde00c9ec3dd6d5d
>        "tools/ocaml: Remove log library from tools/ocaml/libs"
>      (which is in 4.2 but not 4.1)
>
>      Fix this by using the 4.1.x idiom in the new error handling introduced
>      in 070ab4c50593.
>
>      Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> diff --git a/tools/ocaml/xenstored/process.ml
b/tools/ocaml/xenstored/process.ml
> index bd87646..5c81755 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -372,8 +372,8 @@ let do_input store cons doms con >   		try
>   			Connection.do_input con
>   		with Failure exp ->
> -			error "caught exception %s" exp;
> -			error "got a bad client %s" (sprintf "%-8s"
(Connection.get_domstr con));
> +			Logs.error "general" "caught exception %s" exp;
> +			Logs.error "general" "got a bad client %s" (sprintf
"%-8s" (Connection.get_domstr con));
>   			Connection.mark_as_bad con;
>   			false
>   	in
This looks good to me:

Acked-by: David Scott <dave.scott@eu.citrix.com>

Cheers,
Dave

David Scott writes ("Re: [PATCH] Fix ocaml build in 4.1; problem with 4.1.6
release"):
> > commit 7e792ffe54adc2d9fcc210baa8140f210a841c31
> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date:   Mon Sep 9 15:30:42 2013 +0100
> >
> >      oxenstored: Fix process.ml build after 070ab4c50593
...
> This looks good to me:
> 
> Acked-by: David Scott <dave.scott@eu.citrix.com>
Great, thanks.  I have applied that to staging-4.1.

Ian.