[PATCH 0/4] xen: more Coverity-inspired tidying.

[PATCH 0/4] xen: more Coverity-inspired tidying.

[Tim Deegan]

These four patches are small cleanups of things that Coverity complains
about.  AFAICT none of them fixes any bugs, but I do think that they make
the code more readable (i.e. I'm not just mangling the code to make
Coverity happy).  

I'm happy to keep these in a private branch until after we branch 4.4.

Cheers,

Tim.

[PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited'

[Tim Deegan]

The old code relied on implictly casting negative numbers to size_t
making a very large limit, which was correct but non-obvious.

Coverity CID 1128575

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/common/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 43dc392..68553bb 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -239,7 +239,7 @@ static char *number(
 static char *string(char *str, char *end, const char *s,
                     int field_width, int precision, int flags)
 {
-    int i, len = strnlen(s, precision);
+    int i, len = (precision < 0) ? strlen(s) : strnlen(s, precision);
 
     if (!(flags & LEFT)) {
         while (len < field_width--) {
-- 
1.8.4.rc3

[PATCH 2/4] x86/shadow: Drop shadow_mode_trap_reads()

[Tim Deegan]

This was never actually implemented, and is confusing coverity.

Coverity CID 1090354

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/shadow/multi.c | 30 ++++--------------------------
 xen/include/asm-x86/shadow.h   |  4 ----
 2 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 3d35537..5c7a7ac 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -692,21 +692,7 @@ _sh_propagate(struct vcpu *v,
                        && (ft == ft_demand_write))
 #endif /* OOS */
                   ) )
-    {
-        if ( shadow_mode_trap_reads(d) )
-        {
-            // if we are trapping both reads & writes, then mark this page
-            // as not present...
-            //
-            sflags &= ~_PAGE_PRESENT;
-        }
-        else
-        {
-            // otherwise, just prevent any writes...
-            //
-            sflags &= ~_PAGE_RW;
-        }
-    }
+        sflags &= ~_PAGE_RW;
 
     // PV guests in 64-bit mode use two different page tables for user vs
     // supervisor permissions, making the guest's _PAGE_USER bit irrelevant.
@@ -3181,18 +3167,10 @@ static int sh_page_fault(struct vcpu *v,
          && !(mfn_is_out_of_sync(gmfn)
               && !(regs->error_code & PFEC_user_mode))
 #endif
-         )
+         && (ft == ft_demand_write) )
     {
-        if ( ft == ft_demand_write )
-        {
-            perfc_incr(shadow_fault_emulate_write);
-            goto emulate;
-        }
-        else if ( shadow_mode_trap_reads(d) && ft == ft_demand_read )
-        {
-            perfc_incr(shadow_fault_emulate_read);
-            goto emulate;
-        }
+        perfc_incr(shadow_fault_emulate_write);
+        goto emulate;
     }
 
     /* Need to hand off device-model MMIO to the device model */
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 852023d..59edb5f 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -44,10 +44,6 @@
 #define shadow_mode_external(_d)  (paging_mode_shadow(_d) && \
                                    paging_mode_external(_d))
 
-/* Xen traps & emulates all reads of all page table pages:
- * not yet supported */
-#define shadow_mode_trap_reads(_d) ({ (void)(_d); 0; })
-
 /*****************************************************************************
  * Entry points into the shadow code */
 
-- 
1.8.4.rc3

[PATCH 3/4] x86/mem_sharing: drop unused variable.

[Tim Deegan]

Coverity CID 1087198

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/mem_sharing.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 1e89f6c..8355f94 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -833,7 +833,6 @@ int mem_sharing_nominate_page(struct domain *d,
     mfn_t mfn;
     struct page_info *page = NULL; /* gcc... */
     int ret;
-    struct gfn_info *gfn_info;
 
     *phandle = 0UL;
 
@@ -892,7 +891,7 @@ int mem_sharing_nominate_page(struct domain *d,
     page->sharing->handle = get_next_handle();  
 
     /* Create the local gfn info */
-    if ( (gfn_info = mem_sharing_gfn_alloc(page, d, gfn)) == NULL )
+    if ( mem_sharing_gfn_alloc(page, d, gfn) == NULL )
     {
         xfree(page->sharing);
         page->sharing = NULL;
-- 
1.8.4.rc3

[PATCH 4/4] bitmaps/bitops: Clarify tests for small constant size.

[Tim Deegan]

No semantic changes, just makes the control flow a bit clearer.

I was looking at this bcause the (-!__builtin_constant_p(x) | x__)
formula is too clever for Coverity, but in fact it always takes me a
minute or two to understand it too. :)

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/include/asm-x86/bitops.h | 60 +++++++++++++++++++-------------------------
 xen/include/xen/bitmap.h     | 30 +++++++++++++---------
 2 files changed, 44 insertions(+), 46 deletions(-)

diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h
index ab21d92..810beb1 100644
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -335,23 +335,19 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max)
  * @offset: The bitnumber to start searching at
  * @size: The maximum size to search
  */
-#define find_next_bit(addr, size, off) ({ \
-    unsigned int r__ = (size); \
-    unsigned int o__ = (off); \
-    switch ( -!__builtin_constant_p(size) | r__ ) \
-    { \
-    case 0: (void)(addr); break; \
-    case 1 ... BITS_PER_LONG: \
-        r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, r__); \
-        break; \
-    default: \
-        if ( __builtin_constant_p(off) && !o__ ) \
-            r__ = __find_first_bit(addr, r__); \
-        else \
-            r__ = __find_next_bit(addr, r__, o__); \
-        break; \
-    } \
-    r__; \
+#define find_next_bit(addr, size, off) ({                                   \
+    unsigned int r__;                                                       \
+    unsigned int s__ = (size);                                              \
+    unsigned int o__ = (off);                                               \
+    if ( __builtin_constant_p(size) && s__ == 0 )                           \
+        r__ = s__;                                                          \
+    else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG )          \
+        r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, s__);  \
+    else if ( __builtin_constant_p(off) && !o__ )                           \
+        r__ = __find_first_bit(addr, s__);                                  \
+    else                                                                    \
+        r__ = __find_next_bit(addr, s__, o__);                              \
+    r__;                                                                    \
 })
 
 /**
@@ -370,23 +366,19 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max)
  * @offset: The bitnumber to start searching at
  * @size: The maximum size to search
  */
-#define find_next_zero_bit(addr, size, off) ({ \
-    unsigned int r__ = (size); \
-    unsigned int o__ = (off); \
-    switch ( -!__builtin_constant_p(size) | r__ ) \
-    { \
-    case 0: (void)(addr); break; \
-    case 1 ... BITS_PER_LONG: \
-        r__ = o__ + __scanbit(~*(const unsigned long *)(addr) >> o__, r__); \
-        break; \
-    default: \
-        if ( __builtin_constant_p(off) && !o__ ) \
-            r__ = __find_first_zero_bit(addr, r__); \
-        else \
-            r__ = __find_next_zero_bit(addr, r__, o__); \
-        break; \
-    } \
-    r__; \
+#define find_next_zero_bit(addr, size, off) ({                              \
+    unsigned int r__;                                                       \
+    unsigned int s__ = (size);                                              \
+    unsigned int o__ = (off);                                               \
+    if ( __builtin_constant_p(size) && s__ == 0 )                           \
+        r__ = s__;                                                          \
+    else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG )          \
+        r__ = o__ + __scanbit(~*(const unsigned long *)(addr) >> o__, s__); \
+    else if ( __builtin_constant_p(off) && !o__ )                           \
+        r__ = __find_first_zero_bit(addr, s__);                             \
+    else                                                                    \
+        r__ = __find_next_zero_bit(addr, s__, o__);                         \
+    r__;                                                                    \
 })
 
 /**
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index b5ec455..166e1a0 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -110,13 +110,14 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order);
 
 #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
 
-#define bitmap_switch(nbits, zero_ret, small, large)			\
-	switch (-!__builtin_constant_p(nbits) | (nbits)) {		\
-	case 0:	return zero_ret;					\
-	case 1 ... BITS_PER_LONG:					\
-		small; break;						\
-	default:							\
-		large; break;						\
+#define bitmap_switch(nbits, zero, small, large)			  \
+	unsigned int n__ = (nbits);					  \
+	if (__builtin_constant_p(nbits) && n__ == 0) {			  \
+		zero;							  \
+	} else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \
+		small;							  \
+	} else {							  \
+		large;							  \
 	}
 
 static inline void bitmap_zero(unsigned long *dst, int nbits)
@@ -191,7 +192,8 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
 static inline int bitmap_equal(const unsigned long *src1,
 			const unsigned long *src2, int nbits)
 {
-	bitmap_switch(nbits, -1,
+	bitmap_switch(nbits,
+		return -1,
 		return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits)),
 		return __bitmap_equal(src1, src2, nbits));
 }
@@ -199,7 +201,8 @@ static inline int bitmap_equal(const unsigned long *src1,
 static inline int bitmap_intersects(const unsigned long *src1,
 			const unsigned long *src2, int nbits)
 {
-	bitmap_switch(nbits, -1,
+	bitmap_switch(nbits,
+		return -1,
 		return ((*src1 & *src2) & BITMAP_LAST_WORD_MASK(nbits)) != 0,
 		return __bitmap_intersects(src1, src2, nbits));
 }
@@ -207,21 +210,24 @@ static inline int bitmap_intersects(const unsigned long *src1,
 static inline int bitmap_subset(const unsigned long *src1,
 			const unsigned long *src2, int nbits)
 {
-	bitmap_switch(nbits, -1,
+	bitmap_switch(nbits,
+		return -1,
 		return !((*src1 & ~*src2) & BITMAP_LAST_WORD_MASK(nbits)),
 		return __bitmap_subset(src1, src2, nbits));
 }
 
 static inline int bitmap_empty(const unsigned long *src, int nbits)
 {
-	bitmap_switch(nbits, -1,
+	bitmap_switch(nbits,
+		return -1,
 		return !(*src & BITMAP_LAST_WORD_MASK(nbits)),
 		return __bitmap_empty(src, nbits));
 }
 
 static inline int bitmap_full(const unsigned long *src, int nbits)
 {
-	bitmap_switch(nbits, -1,
+	bitmap_switch(nbits,
+		return -1,
 		return !(~*src & BITMAP_LAST_WORD_MASK(nbits)),
 		return __bitmap_full(src, nbits));
 }
-- 
1.8.4.rc3

Re: [PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited'

[Andrew Cooper]

On 28/11/13 16:37, Tim Deegan wrote:
> The old code relied on implictly casting negative numbers to size_t
> making a very large limit, which was correct but non-obvious.
>
> Coverity CID 1128575
>
> Signed-off-by: Tim Deegan <tim@xen.org>

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

This CID was introduced as a side effect of my %ps/%pS series, which was
basically code motion for this piece.  The previous code was not exactly
fantastic.

> ---
>  xen/common/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index 43dc392..68553bb 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -239,7 +239,7 @@ static char *number(
>  static char *string(char *str, char *end, const char *s,
>                      int field_width, int precision, int flags)
>  {
> -    int i, len = strnlen(s, precision);
> +    int i, len = (precision < 0) ? strlen(s) : strnlen(s, precision);
>  
>      if (!(flags & LEFT)) {
>          while (len < field_width--) {

Re: [PATCH 3/4] x86/mem_sharing: drop unused variable.

[Andrew Cooper]

On 28/11/13 16:37, Tim Deegan wrote:
> Coverity CID 1087198
>
> Signed-off-by: Tim Deegan <tim@xen.org>

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

> ---
>  xen/arch/x86/mm/mem_sharing.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 1e89f6c..8355f94 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -833,7 +833,6 @@ int mem_sharing_nominate_page(struct domain *d,
>      mfn_t mfn;
>      struct page_info *page = NULL; /* gcc... */
>      int ret;
> -    struct gfn_info *gfn_info;
>  
>      *phandle = 0UL;
>  
> @@ -892,7 +891,7 @@ int mem_sharing_nominate_page(struct domain *d,
>      page->sharing->handle = get_next_handle();  
>  
>      /* Create the local gfn info */
> -    if ( (gfn_info = mem_sharing_gfn_alloc(page, d, gfn)) == NULL )
> +    if ( mem_sharing_gfn_alloc(page, d, gfn) == NULL )
>      {
>          xfree(page->sharing);
>          page->sharing = NULL;

Re: [PATCH 4/4] bitmaps/bitops: Clarify tests for small constant size.

[Jan Beulich]

>>> On 28.11.13 at 17:37, Tim Deegan <tim@xen.org> wrote:
> No semantic changes, just makes the control flow a bit clearer.
> 
> I was looking at this bcause the (-!__builtin_constant_p(x) | x__)
> formula is too clever for Coverity, but in fact it always takes me a
> minute or two to understand it too. :)

I nevertheless like things like this... Anyway - did you check the
generated code is no worse with this change?

> -#define find_next_bit(addr, size, off) ({ \
> -    unsigned int r__ = (size); \
> -    unsigned int o__ = (off); \
> -    switch ( -!__builtin_constant_p(size) | r__ ) \
> -    { \
> -    case 0: (void)(addr); break; \

This dummy evaluation of addr ...

> -    case 1 ... BITS_PER_LONG: \
> -        r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, r__); \
> -        break; \
> -    default: \
> -        if ( __builtin_constant_p(off) && !o__ ) \
> -            r__ = __find_first_bit(addr, r__); \
> -        else \
> -            r__ = __find_next_bit(addr, r__, o__); \
> -        break; \
> -    } \
> -    r__; \
> +#define find_next_bit(addr, size, off) ({                                   \
> +    unsigned int r__;                                                       \
> +    unsigned int s__ = (size);                                              \
> +    unsigned int o__ = (off);                                               \
> +    if ( __builtin_constant_p(size) && s__ == 0 )                           \
> +        r__ = s__;                                                          \

... is being lost here.

> +    else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG )          \
> +        r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, s__);  \
> +    else if ( __builtin_constant_p(off) && !o__ )                           \
> +        r__ = __find_first_bit(addr, s__);                                  \
> +    else                                                                    \
> +        r__ = __find_next_bit(addr, s__, o__);                              \
> +    r__;                                                                    \

Jan

Re: [PATCH 4/4] bitmaps/bitops: Clarify tests for small constant size.

[Tim Deegan]

At 10:07 +0000 on 29 Nov (1385716068), Jan Beulich wrote:
> >>> On 28.11.13 at 17:37, Tim Deegan <tim@xen.org> wrote:
> > No semantic changes, just makes the control flow a bit clearer.
> > 
> > I was looking at this bcause the (-!__builtin_constant_p(x) | x__)
> > formula is too clever for Coverity, but in fact it always takes me a
> > minute or two to understand it too. :)
> 
> I nevertheless like things like this...

Yes, it's a nice trick, but I think we're better off with the
readable code.

> Anyway - did you check the
> generated code is no worse with this change?

Yep, the generated code is identical.

> > -#define find_next_bit(addr, size, off) ({ \
> > -    unsigned int r__ = (size); \
> > -    unsigned int o__ = (off); \
> > -    switch ( -!__builtin_constant_p(size) | r__ ) \
> > -    { \
> > -    case 0: (void)(addr); break; \
> 
> This dummy evaluation of addr ...

Oops, yep.  I'll fix for v2.

Tim.

Re: [PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited'

[Ian Campbell]

On Thu, 2013-11-28 at 16:48 +0000, Andrew Cooper wrote:
> On 28/11/13 16:37, Tim Deegan wrote:
> > The old code relied on implictly casting negative numbers to size_t
> > making a very large limit, which was correct but non-obvious.
> >
> > Coverity CID 1128575
> >
> > Signed-off-by: Tim Deegan <tim@xen.org>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> This CID was introduced as a side effect of my %ps/%pS series, which was
> basically code motion for this piece.  The previous code was not exactly
> fantastic.

I wonder if we should therefore take this for 4.4?