* [PATCH v2 0/4] more Coverity-inspired tidying.
@ 2014-02-27 14:27 Tim Deegan
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
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).
Reposting now that 4.4 has branched, with a v2 of patch 4/4 fixing
a missing argument evaluation.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited'
2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan
@ 2014-02-27 14:27 ` Tim Deegan
2014-02-28 15:07 ` Keir Fraser
2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
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>
---
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 1a6198e..0a6fa05 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.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads()
2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
@ 2014-02-27 14:27 ` Tim Deegan
2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan
2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan
3 siblings, 0 replies; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
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 348915e..f40cab4 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.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] x86/mem_sharing: drop unused variable.
2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan
@ 2014-02-27 14:27 ` Tim Deegan
2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan
3 siblings, 0 replies; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
Coverity CID 1087198
Signed-off-by: Tim Deegan <tim@xen.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.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 4a5d9e8..7ed6594 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -846,7 +846,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;
@@ -905,7 +904,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.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size.
2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan
` (2 preceding siblings ...)
2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan
@ 2014-02-27 14:27 ` Tim Deegan
2014-02-27 16:30 ` Jan Beulich
3 siblings, 1 reply; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
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>
---
v2: fix find_next_bit macros to evaluate 'addr' exactly once.
---
xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------
xen/include/xen/bitmap.h | 30 ++++++++++++---------
2 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h
index ab21d92..05ed2d7 100644
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -335,23 +335,20 @@ 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__; \
+ const unsigned long *a__ = (addr); \
+ 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 *)(a__) >> o__, s__); \
+ else if ( __builtin_constant_p(off) && !o__ ) \
+ r__ = __find_first_bit(a__, s__); \
+ else \
+ r__ = __find_next_bit(a__, s__, o__); \
+ r__; \
})
/**
@@ -370,23 +367,20 @@ 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__; \
+ const unsigned long *a__ = (addr); \
+ 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 *)(a__) >> o__, s__); \
+ else if ( __builtin_constant_p(off) && !o__ ) \
+ r__ = __find_first_zero_bit(a__, s__); \
+ else \
+ r__ = __find_next_zero_bit(a__, 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.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size.
2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan
@ 2014-02-27 16:30 ` Jan Beulich
2014-02-27 16:38 ` Tim Deegan
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2014-02-27 16:30 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel
>>> On 27.02.14 at 15:27, 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. :)
>
> Signed-off-by: Tim Deegan <tim@xen.org>
Well, I'm not really happy to see this changed into more text
(even if fewer lines), because to me that's part of what makes
such macros badly readable, but ...
> xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------
> xen/include/xen/bitmap.h | 30 ++++++++++++---------
> 2 files changed, 46 insertions(+), 46 deletions(-)
... at least the overall change is not growing the number of
lines.
Nevertheless a small consistency nit:
> +#define find_next_bit(addr, size, off) ({ \
> + unsigned int r__; \
> + const unsigned long *a__ = (addr); \
> + unsigned int s__ = (size); \
> + unsigned int o__ = (off); \
> + if ( __builtin_constant_p(size) && s__ == 0 ) \
Using == 0 here, ...
> + r__ = s__; \
> + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
> + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \
> + else if ( __builtin_constant_p(off) && !o__ ) \
... but using ! here. I'd prefer the latter everywhere, but I'd also
be fine with you choosing the former consistently. (Same of course
again further down.)
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size.
2014-02-27 16:30 ` Jan Beulich
@ 2014-02-27 16:38 ` Tim Deegan
2014-02-28 14:40 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 16:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
At 16:30 +0000 on 27 Feb (1393515036), Jan Beulich wrote:
> Using == 0 here, ...
>
> > + r__ = s__; \
> > + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
> > + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \
> > + else if ( __builtin_constant_p(off) && !o__ ) \
>
> ... but using ! here. I'd prefer the latter everywhere, but I'd also
> be fine with you choosing the former consistently. (Same of course
> again further down.)
Good point. v3 is below
Tim.
>From dba941064825d723b79f866d16bb0f07585de320 Mon Sep 17 00:00:00 2001
From: Tim Deegan <tim@xen.org>
Date: Thu, 28 Nov 2013 15:40:48 +0000
Subject: [PATCH] bitmaps/bitops: Clarify tests for small constant size.
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>
---
v3: Consistenly use '!foo' rather than 'foo == 0'
v2: fix find_next_bit macros to evaluate 'addr' exactly once.
---
xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------
xen/include/xen/bitmap.h | 30 ++++++++++++---------
2 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h
index ab21d92..82a08ee 100644
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -335,23 +335,20 @@ 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__; \
+ const unsigned long *a__ = (addr); \
+ unsigned int s__ = (size); \
+ unsigned int o__ = (off); \
+ if ( __builtin_constant_p(size) && !s__ ) \
+ r__ = s__; \
+ else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
+ r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \
+ else if ( __builtin_constant_p(off) && !o__ ) \
+ r__ = __find_first_bit(a__, s__); \
+ else \
+ r__ = __find_next_bit(a__, s__, o__); \
+ r__; \
})
/**
@@ -370,23 +367,20 @@ 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__; \
+ const unsigned long *a__ = (addr); \
+ unsigned int s__ = (size); \
+ unsigned int o__ = (off); \
+ if ( __builtin_constant_p(size) && !s__ ) \
+ r__ = s__; \
+ else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
+ r__ = o__ + __scanbit(~*(const unsigned long *)(a__) >> o__, s__); \
+ else if ( __builtin_constant_p(off) && !o__ ) \
+ r__ = __find_first_zero_bit(a__, s__); \
+ else \
+ r__ = __find_next_zero_bit(a__, s__, o__); \
+ r__; \
})
/**
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index b5ec455..e2a3686 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__) { \
+ 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.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size.
2014-02-27 16:38 ` Tim Deegan
@ 2014-02-28 14:40 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2014-02-28 14:40 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel
>>> On 27.02.14 at 17:38, Tim Deegan <tim@xen.org> wrote:
> From dba941064825d723b79f866d16bb0f07585de320 Mon Sep 17 00:00:00 2001
> From: Tim Deegan <tim@xen.org>
> Date: Thu, 28 Nov 2013 15:40:48 +0000
> Subject: [PATCH] bitmaps/bitops: Clarify tests for small constant size.
>
> 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>
A little reluctantly
Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
>
> v3: Consistenly use '!foo' rather than 'foo == 0'
> v2: fix find_next_bit macros to evaluate 'addr' exactly once.
> ---
> xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------
> xen/include/xen/bitmap.h | 30 ++++++++++++---------
> 2 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h
> index ab21d92..82a08ee 100644
> --- a/xen/include/asm-x86/bitops.h
> +++ b/xen/include/asm-x86/bitops.h
> @@ -335,23 +335,20 @@ 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__;
> \
> + const unsigned long *a__ = (addr);
> \
> + unsigned int s__ = (size);
> \
> + unsigned int o__ = (off);
> \
> + if ( __builtin_constant_p(size) && !s__ ) \
> + r__ = s__;
> \
> + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
> + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \
> + else if ( __builtin_constant_p(off) && !o__ ) \
> + r__ = __find_first_bit(a__, s__);
> \
> + else
> \
> + r__ = __find_next_bit(a__, s__, o__);
> \
> + r__;
> \
> })
>
> /**
> @@ -370,23 +367,20 @@ 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__;
> \
> + const unsigned long *a__ = (addr);
> \
> + unsigned int s__ = (size);
> \
> + unsigned int o__ = (off);
> \
> + if ( __builtin_constant_p(size) && !s__ ) \
> + r__ = s__;
> \
> + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \
> + r__ = o__ + __scanbit(~*(const unsigned long *)(a__) >> o__, s__); \
> + else if ( __builtin_constant_p(off) && !o__ ) \
> + r__ = __find_first_zero_bit(a__, s__);
> \
> + else
> \
> + r__ = __find_next_zero_bit(a__, s__, o__);
> \
> + r__;
> \
> })
>
> /**
> diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
> index b5ec455..e2a3686 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__) { \
> + 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.5.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited'
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
@ 2014-02-28 15:07 ` Keir Fraser
0 siblings, 0 replies; 9+ messages in thread
From: Keir Fraser @ 2014-02-28 15:07 UTC (permalink / raw)
To: Tim Deegan; +Cc: Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1128 bytes --]
On Thu, Feb 27, 2014 at 2:27 PM, Tim Deegan <tim@xen.org> 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>
>
Acked-by: Keir Fraser <keir@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 1a6198e..0a6fa05 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.5.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
[-- Attachment #1.2: Type: text/html, Size: 1968 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-02-28 15:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
2014-02-28 15:07 ` Keir Fraser
2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan
2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan
2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan
2014-02-27 16:30 ` Jan Beulich
2014-02-27 16:38 ` Tim Deegan
2014-02-28 14:40 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.