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.
Tim Deegan
2013-Nov-28 16:37 UTC
[PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as ''unlimited''
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
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
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
Tim Deegan
2013-Nov-28 16:37 UTC
[PATCH 4/4] 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>
---
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
Andrew Cooper
2013-Nov-28 16:48 UTC
Re: [PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as ''unlimited''
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--) {
Andrew Cooper
2013-Nov-28 16:50 UTC
Re: [PATCH 3/4] x86/mem_sharing: drop unused variable.
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;
Jan Beulich
2013-Nov-29 10:07 UTC
Re: [PATCH 4/4] bitmaps/bitops: Clarify tests for small constant size.
>>> 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
Tim Deegan
2013-Nov-29 10:37 UTC
Re: [PATCH 4/4] bitmaps/bitops: Clarify tests for small constant size.
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.
Ian Campbell
2013-Nov-29 10:45 UTC
Re: [PATCH 1/4] common/vsprintf: Explicitly treat negative lengths as ''unlimited''
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?