[PATCH] Fix ocaml build in 4.1; problem with 4.1.6 release

[PATCH] Fix ocaml build in 4.1; problem with 4.1.6 release

[Ian Jackson]

(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

Re: Problem with 4.1.6 release [and 1 more messages]

[Ian Jackson]

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.

Re: Problem with 4.1.6 release [and 1 more messages]

[Jan Beulich]

>>> 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

Re: [PATCH] Fix ocaml build in 4.1; problem with 4.1.6 release

[David Scott]

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

Re: [PATCH] Fix ocaml build in 4.1; problem with 4.1.6 release

[Ian Jackson]

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.