| Message ID | 1455798403-21953-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.02.16 at 13:26, <andrew.cooper3@citrix.com> wrote: > Coverity objects otherwise. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > --- > xen/arch/x86/mm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index a05edc3..0bff7dd 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -924,10 +924,15 @@ get_page_from_l1e( > { > case 0: > break; > + > case 1: > if ( is_hardware_domain(l1e_owner) ) > + { > + /* Fallthrough. */ > case -1: > return 0; > + } > + /* Fallthrough. */ > default: This second fall-through is actually a bug (luckily noticable only on debug builds). I'll commit the patch suitably adjusted, albeit I have a hard time seeing how case 1: if ( is_hardware_domain(l1e_owner) ) case -1: cannot be seen as obviously deliberate. Or did Coverity perhaps only complain about the second, indeed buggy one? Jan
On 18/02/16 13:38, Jan Beulich wrote: >>>> On 18.02.16 at 13:26, <andrew.cooper3@citrix.com> wrote: >> Coverity objects otherwise. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> --- >> xen/arch/x86/mm.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c >> index a05edc3..0bff7dd 100644 >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -924,10 +924,15 @@ get_page_from_l1e( >> { >> case 0: >> break; >> + >> case 1: >> if ( is_hardware_domain(l1e_owner) ) >> + { >> + /* Fallthrough. */ >> case -1: >> return 0; >> + } >> + /* Fallthrough. */ >> default: > This second fall-through is actually a bug (luckily noticable only > on debug builds). > > I'll commit the patch suitably adjusted, albeit I have a hard time > seeing how > > case 1: > if ( is_hardware_domain(l1e_owner) ) > case -1: > > cannot be seen as obviously deliberate. Many C programmers are not aware that it is even valid syntax. The point of the MISSING_BREAK check is to second guess what the programmer has actually written. > Or did Coverity perhaps > only complain about the second, indeed buggy one? It only complained about the first. The second is a misaligned unconditional return when considered in isolation. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index a05edc3..0bff7dd 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -924,10 +924,15 @@ get_page_from_l1e( { case 0: break; + case 1: if ( is_hardware_domain(l1e_owner) ) + { + /* Fallthrough. */ case -1: return 0; + } + /* Fallthrough. */ default: ASSERT_UNREACHABLE(); }
Coverity objects otherwise. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/mm.c | 5 +++++ 1 file changed, 5 insertions(+)