[PATCH 1/2] xen/flask: Rename cond_expr.bool to bool_val

[PATCH 1/2] xen/flask: Rename cond_expr.bool to bool_val

[Andrew Cooper]

A subsequent change will introduce C99 bools, at which point 'bool'
becomes a type, and ineligible as a variable name.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/xsm/flask/ss/conditional.c | 6 +++---
 xen/xsm/flask/ss/conditional.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/xsm/flask/ss/conditional.c b/xen/xsm/flask/ss/conditional.c
index 098ddc0..3e58aea 100644
--- a/xen/xsm/flask/ss/conditional.c
+++ b/xen/xsm/flask/ss/conditional.c
@@ -40,7 +40,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
                 if ( sp == (COND_EXPR_MAXDEPTH - 1) )
                     return -1;
                 sp++;
-                s[sp] = p->bool_val_to_struct[cur->bool - 1]->state;
+                s[sp] = p->bool_val_to_struct[cur->bool_val - 1]->state;
             break;
             case COND_NOT:
                 if ( sp < 0 )
@@ -404,7 +404,7 @@ static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
         return 0;
     }
 
-    if ( expr->bool > p->p_bools.nprim )
+    if ( expr->bool_val > p->p_bools.nprim )
     {
         printk("Flask: conditional expressions uses unknown bool.\n");
         return 0;
@@ -444,7 +444,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
             goto err;
 
         expr->expr_type = le32_to_cpu(buf[0]);
-        expr->bool = le32_to_cpu(buf[1]);
+        expr->bool_val = le32_to_cpu(buf[1]);
 
         if ( !expr_isvalid(p, expr) )
         {
diff --git a/xen/xsm/flask/ss/conditional.h b/xen/xsm/flask/ss/conditional.h
index d389ecf..59ac6b4 100644
--- a/xen/xsm/flask/ss/conditional.h
+++ b/xen/xsm/flask/ss/conditional.h
@@ -30,7 +30,7 @@ struct cond_expr {
 #define COND_NEQ    7 /* bool != bool */
 #define COND_LAST    COND_NEQ
     __u32 expr_type;
-    __u32 bool;
+    __u32 bool_val;
     struct cond_expr *next;
 };
 
-- 
2.1.4


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

[PATCH 2/2] xen/build: Use C99 booleans

[Andrew Cooper]

and switch bool_t to being of type _Bool rather than char.

Using bool_t as char causes several subtle problems; first that a bool_t
actually has more than two values, and that (bool_t)0x100 actually has the
value 0 rather than the expected 1, due to truncation.

Making this change reveals two bugs now caught by the compiler.
errata_c6_eoi_workaround() actually makes use of bool_t having more than two
states, while generic_apic_probe() has a integer in the middle of a compound
bool_t assignment (which triggers a [-Werror=parentheses] warning on Debian
Jessie).

Finally, it turns out that ARM is mixing and matching bool_t and bool, despite
their different semantics.  This change brings the semantics of bool_t to
match bool, but does not alter the current mix.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>

v2:
 * Leave the matter of std libaries to one side for now.  Fixing bool_t is far
   more important, and can be done without making the std include issue any
   worse.
 * Type tweaks, per v1 review.
---
 xen/arch/arm/p2m.c                   | 1 -
 xen/arch/arm/platforms/xgene-storm.c | 1 -
 xen/arch/arm/traps.c                 | 1 -
 xen/arch/x86/acpi/cpu_idle.c         | 2 +-
 xen/arch/x86/genapic/probe.c         | 3 ++-
 xen/include/asm-arm/types.h          | 4 ----
 xen/include/asm-x86/types.h          | 4 ----
 xen/include/xen/device_tree.h        | 1 -
 xen/include/xen/types.h              | 6 ++++++
 9 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 976f97b..a4bc55a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1,7 +1,6 @@
 #include <xen/config.h>
 #include <xen/sched.h>
 #include <xen/lib.h>
-#include <xen/stdbool.h>
 #include <xen/errno.h>
 #include <xen/domain_page.h>
 #include <xen/bitops.h>
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 70cb655..686b19b 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -20,7 +20,6 @@
 
 #include <xen/config.h>
 #include <asm/platform.h>
-#include <xen/stdbool.h>
 #include <xen/vmap.h>
 #include <xen/device_tree.h>
 #include <asm/io.h>
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3326122..a2eb1da 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -17,7 +17,6 @@
  */
 
 #include <xen/config.h>
-#include <xen/stdbool.h>
 #include <xen/init.h>
 #include <xen/string.h>
 #include <xen/version.h>
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index a21aeed..7e235a3 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -480,7 +480,7 @@ void trace_exit_reason(u32 *irq_traced)
  */
 bool_t errata_c6_eoi_workaround(void)
 {
-    static bool_t fix_needed = -1;
+    static int8_t fix_needed = -1;
 
     if ( unlikely(fix_needed == -1) )
     {
diff --git a/xen/arch/x86/genapic/probe.c b/xen/arch/x86/genapic/probe.c
index a5f2a24..860201e 100644
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -56,7 +56,8 @@ custom_param("apic", genapic_apic_force);
 
 void __init generic_apic_probe(void) 
 { 
-	int i, changed;
+	bool changed;
+	int i;
 
 	record_boot_APIC_mode();
 
diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h
index 09e5455..71d2e42 100644
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -62,10 +62,6 @@ typedef unsigned long size_t;
 #endif
 typedef signed long ssize_t;
 
-typedef char bool_t;
-#define test_and_set_bool(b)   xchg(&(b), 1)
-#define test_and_clear_bool(b) xchg(&(b), 0)
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ARM_TYPES_H__ */
diff --git a/xen/include/asm-x86/types.h b/xen/include/asm-x86/types.h
index b82fa58..e75b744 100644
--- a/xen/include/asm-x86/types.h
+++ b/xen/include/asm-x86/types.h
@@ -41,10 +41,6 @@ typedef unsigned long size_t;
 #endif
 typedef signed long ssize_t;
 
-typedef char bool_t;
-#define test_and_set_bool(b)   xchg(&(b), 1)
-#define test_and_clear_bool(b) xchg(&(b), 0)
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* __X86_TYPES_H__ */
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index d7d1b40..3657ac2 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -17,7 +17,6 @@
 #include <xen/init.h>
 #include <xen/string.h>
 #include <xen/types.h>
-#include <xen/stdbool.h>
 #include <xen/list.h>
 
 #define DEVICE_TREE_MAX_DEPTH 16
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 8596ded..78410de 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -1,6 +1,8 @@
 #ifndef __TYPES_H__
 #define __TYPES_H__
 
+#include <xen/stdbool.h>
+
 #include <asm/types.h>
 
 #define BITS_TO_LONGS(bits) \
@@ -59,4 +61,8 @@ typedef __u64 __be64;
 
 typedef unsigned long uintptr_t;
 
+typedef _Bool bool_t;
+#define test_and_set_bool(b)   xchg(&(b), true)
+#define test_and_clear_bool(b) xchg(&(b), false)
+
 #endif /* __TYPES_H__ */
-- 
2.1.4


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

Re: [PATCH 2/2] xen/build: Use C99 booleans

[Julien Grall]

Hi Andrew,

On 14/07/16 16:58, Andrew Cooper wrote:
> and switch bool_t to being of type _Bool rather than char.
>
> Using bool_t as char causes several subtle problems; first that a bool_t
> actually has more than two values, and that (bool_t)0x100 actually has the
> value 0 rather than the expected 1, due to truncation.
>
> Making this change reveals two bugs now caught by the compiler.
> errata_c6_eoi_workaround() actually makes use of bool_t having more than two
> states, while generic_apic_probe() has a integer in the middle of a compound
> bool_t assignment (which triggers a [-Werror=parentheses] warning on Debian
> Jessie).
>
> Finally, it turns out that ARM is mixing and matching bool_t and bool, despite
> their different semantics.  This change brings the semantics of bool_t to
> match bool, but does not alter the current mix.

I will add an item in my todo list to clean-up the ARM code. Is there 
any plan to retire either bool_t or bool?

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

 From ARM bits:

Acked-by: Julien Grall <julien.grall@arm.com>

> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> v2:
>   * Leave the matter of std libaries to one side for now.  Fixing bool_t is far
>     more important, and can be done without making the std include issue any
>     worse.
>   * Type tweaks, per v1 review.
> ---
>   xen/arch/arm/p2m.c                   | 1 -
>   xen/arch/arm/platforms/xgene-storm.c | 1 -
>   xen/arch/arm/traps.c                 | 1 -
>   xen/arch/x86/acpi/cpu_idle.c         | 2 +-
>   xen/arch/x86/genapic/probe.c         | 3 ++-
>   xen/include/asm-arm/types.h          | 4 ----
>   xen/include/asm-x86/types.h          | 4 ----
>   xen/include/xen/device_tree.h        | 1 -
>   xen/include/xen/types.h              | 6 ++++++
>   9 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 976f97b..a4bc55a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1,7 +1,6 @@
>   #include <xen/config.h>
>   #include <xen/sched.h>
>   #include <xen/lib.h>
> -#include <xen/stdbool.h>
>   #include <xen/errno.h>
>   #include <xen/domain_page.h>
>   #include <xen/bitops.h>
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 70cb655..686b19b 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -20,7 +20,6 @@
>
>   #include <xen/config.h>
>   #include <asm/platform.h>
> -#include <xen/stdbool.h>
>   #include <xen/vmap.h>
>   #include <xen/device_tree.h>
>   #include <asm/io.h>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 3326122..a2eb1da 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -17,7 +17,6 @@
>    */
>
>   #include <xen/config.h>
> -#include <xen/stdbool.h>
>   #include <xen/init.h>
>   #include <xen/string.h>
>   #include <xen/version.h>
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index a21aeed..7e235a3 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -480,7 +480,7 @@ void trace_exit_reason(u32 *irq_traced)
>    */
>   bool_t errata_c6_eoi_workaround(void)
>   {
> -    static bool_t fix_needed = -1;
> +    static int8_t fix_needed = -1;
>
>       if ( unlikely(fix_needed == -1) )
>       {
> diff --git a/xen/arch/x86/genapic/probe.c b/xen/arch/x86/genapic/probe.c
> index a5f2a24..860201e 100644
> --- a/xen/arch/x86/genapic/probe.c
> +++ b/xen/arch/x86/genapic/probe.c
> @@ -56,7 +56,8 @@ custom_param("apic", genapic_apic_force);
>
>   void __init generic_apic_probe(void)
>   {
> -	int i, changed;
> +	bool changed;
> +	int i;
>
>   	record_boot_APIC_mode();
>
> diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h
> index 09e5455..71d2e42 100644
> --- a/xen/include/asm-arm/types.h
> +++ b/xen/include/asm-arm/types.h
> @@ -62,10 +62,6 @@ typedef unsigned long size_t;
>   #endif
>   typedef signed long ssize_t;
>
> -typedef char bool_t;
> -#define test_and_set_bool(b)   xchg(&(b), 1)
> -#define test_and_clear_bool(b) xchg(&(b), 0)
> -
>   #endif /* __ASSEMBLY__ */
>
>   #endif /* __ARM_TYPES_H__ */
> diff --git a/xen/include/asm-x86/types.h b/xen/include/asm-x86/types.h
> index b82fa58..e75b744 100644
> --- a/xen/include/asm-x86/types.h
> +++ b/xen/include/asm-x86/types.h
> @@ -41,10 +41,6 @@ typedef unsigned long size_t;
>   #endif
>   typedef signed long ssize_t;
>
> -typedef char bool_t;
> -#define test_and_set_bool(b)   xchg(&(b), 1)
> -#define test_and_clear_bool(b) xchg(&(b), 0)
> -
>   #endif /* __ASSEMBLY__ */
>
>   #endif /* __X86_TYPES_H__ */
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index d7d1b40..3657ac2 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -17,7 +17,6 @@
>   #include <xen/init.h>
>   #include <xen/string.h>
>   #include <xen/types.h>
> -#include <xen/stdbool.h>
>   #include <xen/list.h>
>
>   #define DEVICE_TREE_MAX_DEPTH 16
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 8596ded..78410de 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -1,6 +1,8 @@
>   #ifndef __TYPES_H__
>   #define __TYPES_H__
>
> +#include <xen/stdbool.h>
> +
>   #include <asm/types.h>
>
>   #define BITS_TO_LONGS(bits) \
> @@ -59,4 +61,8 @@ typedef __u64 __be64;
>
>   typedef unsigned long uintptr_t;
>
> +typedef _Bool bool_t;
> +#define test_and_set_bool(b)   xchg(&(b), true)
> +#define test_and_clear_bool(b) xchg(&(b), false)
> +
>   #endif /* __TYPES_H__ */
>

-- 
Julien Grall

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

Re: [PATCH 2/2] xen/build: Use C99 booleans

[Andrew Cooper]

On 14/07/16 17:12, Julien Grall wrote:
> Hi Andrew,
>
> On 14/07/16 16:58, Andrew Cooper wrote:
>> and switch bool_t to being of type _Bool rather than char.
>>
>> Using bool_t as char causes several subtle problems; first that a bool_t
>> actually has more than two values, and that (bool_t)0x100 actually
>> has the
>> value 0 rather than the expected 1, due to truncation.
>>
>> Making this change reveals two bugs now caught by the compiler.
>> errata_c6_eoi_workaround() actually makes use of bool_t having more
>> than two
>> states, while generic_apic_probe() has a integer in the middle of a
>> compound
>> bool_t assignment (which triggers a [-Werror=parentheses] warning on
>> Debian
>> Jessie).
>>
>> Finally, it turns out that ARM is mixing and matching bool_t and
>> bool, despite
>> their different semantics.  This change brings the semantics of
>> bool_t to
>> match bool, but does not alter the current mix.
>
> I will add an item in my todo list to clean-up the ARM code. Is there
> any plan to retire either bool_t or bool?

I would prefer if we start making a trend towards bool, and the use of
true/false where appropriate.  Jan appears to agree; it is consistent
with Linux, and I think its easier to read and understand.

I am not suggesting that we do a bulk replace right now, and it will
take a while for contributors to change their style.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> From ARM bits:
>
> Acked-by: Julien Grall <julien.grall@arm.com>

Thanks,

~Andrew

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

Re: [PATCH 2/2] xen/build: Use C99 booleans

[Tim Deegan]

At 16:58 +0100 on 14 Jul (1468515536), Andrew Cooper wrote:
> and switch bool_t to being of type _Bool rather than char.
> 
> Using bool_t as char causes several subtle problems; first that a bool_t
> actually has more than two values, and that (bool_t)0x100 actually has the
> value 0 rather than the expected 1, due to truncation.
> 
> Making this change reveals two bugs now caught by the compiler.
> errata_c6_eoi_workaround() actually makes use of bool_t having more than two
> states, while generic_apic_probe() has a integer in the middle of a compound
> bool_t assignment (which triggers a [-Werror=parentheses] warning on Debian
> Jessie).
> 
> Finally, it turns out that ARM is mixing and matching bool_t and bool, despite
> their different semantics.  This change brings the semantics of bool_t to
> match bool, but does not alter the current mix.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thank you for doing this!

Acked-by: Tim Deegan <tim@xen.org>

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

Re: [PATCH 1/2] xen/flask: Rename cond_expr.bool to bool_val

[Daniel De Graaf]

On 07/14/2016 11:58 AM, Andrew Cooper wrote:
> A subsequent change will introduce C99 bools, at which point 'bool'
> becomes a type, and ineligible as a variable name.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

Re: [PATCH 2/2] xen/build: Use C99 booleans

[Jan Beulich]

>>> On 14.07.16 at 17:58, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -1,6 +1,8 @@
>  #ifndef __TYPES_H__
>  #define __TYPES_H__
>  
> +#include <xen/stdbool.h>
> +
>  #include <asm/types.h>
>  
>  #define BITS_TO_LONGS(bits) \
> @@ -59,4 +61,8 @@ typedef __u64 __be64;
>  
>  typedef unsigned long uintptr_t;
>  
> +typedef _Bool bool_t;

I see this went in already, but I think it is slightly sub-optimal: Either
you use _Bool here and don't include xen/stdbool.h above, or you
instead use bool in this typedef.

Jan


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

Re: [PATCH 2/2] xen/build: Use C99 booleans

[Andrew Cooper]

On 01/08/16 11:29, Jan Beulich wrote:
>>>> On 14.07.16 at 17:58, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
>> @@ -1,6 +1,8 @@
>>  #ifndef __TYPES_H__
>>  #define __TYPES_H__
>>  
>> +#include <xen/stdbool.h>
>> +
>>  #include <asm/types.h>
>>  
>>  #define BITS_TO_LONGS(bits) \
>> @@ -59,4 +61,8 @@ typedef __u64 __be64;
>>  
>>  typedef unsigned long uintptr_t;
>>  
>> +typedef _Bool bool_t;
> I see this went in already, but I think it is slightly sub-optimal: Either
> you use _Bool here and don't include xen/stdbool.h above, or you
> instead use bool in this typedef.

We should include xen/stdbool.h to get true and false.

Switching this to bool is easy enough.

~Andrew

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