Re: [PATCH] xen/Coverity: Audit of MISSING_BREAK defects

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/15 21:06, Don Koch wrote:
> On Thu, 12 Feb 2015 20:08:46 +0000
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>> Coverity uses several heuristics to identify when one case statement
>> legitimately falls through into the next, and a comment as the final item in a
>> case statement is one heuristic (the assumption being that it is a
>> justification for the fallthrough).
>>
>> Use this to perform an audit of defects and hide the legitimate fallthroughs.
>>
>> No functional change.  All identified fallthroughs are legitimate.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Coverity-IDs: 1055483, 1055484, 1055486 - 1055488, 1055490 - 1055496,
>>               1055498 - 1055500, 1055501, 1220091
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Xen Coverity Team <coverity@xen.org>
>> ---
>>  xen/arch/x86/hvm/emulate.c      |    1 +
>>  xen/arch/x86/hvm/svm/svm.c      |    1 +
>>  xen/arch/x86/hvm/vlapic.c       |    1 +
>>  xen/arch/x86/mm.c               |    2 ++
>>  xen/arch/x86/traps.c            |    3 +++
>>  xen/arch/x86/x86_64/compat/mm.c |    1 +
>>  xen/common/lib.c                |    4 ++++
>>  xen/common/schedule.c           |    1 +
>>  8 files changed, 14 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 636c909..c657bc6 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -161,6 +161,7 @@ static int hvmemul_do_io(
>>                  put_page(ram_page);
>>              return X86EMUL_RETRY;
>>          }
>> +        /* fallthrough */
>>      default:
>>          if ( ram_page )
>>              put_page(ram_page);
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index a7655bd..018dd70 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2378,6 +2378,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>              case NESTEDHVM_VMEXIT_ERROR:
>>                  break;
>>              }
>> +            /* fallthrough */
>>          case NESTEDHVM_VMEXIT_ERROR:
>>              gdprintk(XENLOG_ERR,
>>                  "nestedsvm_check_intercepts() returned NESTEDHVM_VMEXIT_ERROR\n");
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 5da6d8f..cee8699 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -762,6 +762,7 @@ static int vlapic_reg_write(struct vcpu *v,
>>              vlapic->hw.tdt_msr = 0;
>>          }
>>          vlapic->pt.irq = val & APIC_VECTOR_MASK;
>> +        /* fallthrough */
>>      case APIC_LVTTHMR:      /* LVT Thermal Monitor */
>>      case APIC_LVTPC:        /* LVT Performance Counter */
>>      case APIC_LVT0:         /* LVT LINT0 Reg */
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d4965da..12e5006 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2771,6 +2771,7 @@ int new_guest_cr3(unsigned long mfn)
>>              {
>>              case -EINTR:
>>                  rc = -ERESTART;
>> +                /* fallthrough */
>>              case -ERESTART:
>>                  curr->arch.old_guest_table = page;
>>                  break;
>> @@ -3126,6 +3127,7 @@ long do_mmuext_op(
>>                      {
>>                      case -EINTR:
>>                          rc = -ERESTART;
>> +                        /* fallthrough */
>>                      case -ERESTART:
>>                          curr->arch.old_guest_table = page;
>>                          okay = 0;
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index f5516dc..057a7af 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1739,7 +1739,9 @@ static int guest_io_okay(
>>                                            port>>3, 2) )
>>          {
>>          default: x.bytes[0] = ~0;
>> +            /* fallthrough */
>>          case 1:  x.bytes[1] = ~0;
>> +            /* fallthrough */
>>          case 0:  break;
>>          }
>>          TOGGLE_MODE();
>> @@ -3320,6 +3322,7 @@ static void pci_serr_error(const struct cpu_user_regs *regs)
>>      {
>>      case 'd': /* 'dom0' */
>>          nmi_hwdom_report(_XEN_NMIREASON_pci_serr);
>> +        /* fallthrough */
>>      case 'i': /* 'ignore' */
>>          /* Would like to print a diagnostic here but can't call printk()
>>             from NMI context -- raise a softirq instead. */
>> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
>> index f90f611..1491ce3 100644
>> --- a/xen/arch/x86/x86_64/compat/mm.c
>> +++ b/xen/arch/x86/x86_64/compat/mm.c
>> @@ -292,6 +292,7 @@ int compat_mmuext_op(XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t) cmp_uops,
>>                  break;
>>              case MMUEXT_NEW_USER_BASEPTR:
>>                  rc = -EINVAL;
>> +                /* fallthrough */
>>              case MMUEXT_TLB_FLUSH_LOCAL:
>>              case MMUEXT_TLB_FLUSH_MULTI:
>>              case MMUEXT_TLB_FLUSH_ALL:
>> diff --git a/xen/common/lib.c b/xen/common/lib.c
>> index 89c74ad..ae0bbb3 100644
>> --- a/xen/common/lib.c
>> +++ b/xen/common/lib.c
>> @@ -461,12 +461,16 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
>>      {
>>      case 'T': case 't':
>>          ret <<= 10;
>> +        /* fallthrough */
>>      case 'G': case 'g':
>>          ret <<= 10;
>> +        /* fallthrough */
>>      case 'M': case 'm':
>>          ret <<= 10;
>> +        /* fallthrough */
>>      case 'K': case 'k':
>>          ret <<= 10;
>> +        /* fallthrough */
>>      case 'B': case 'b':
>>          s1++;
>>          break;
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index b73177f..ef79847 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1179,6 +1179,7 @@ static void schedule(void)
>>      {
>>      case TASKLET_enqueued:
>>          set_bit(_TASKLET_scheduled, tasklet_work);
>> +        /* fallthrough */
>>      case TASKLET_enqueued|TASKLET_scheduled:
>>          tasklet_work_scheduled = 1;
>>          break;
> I'm surprised that coverity didn't complain about the fallthrough in
> the next case:
>
>     case TASKLET_scheduled:
>         clear_bit(_TASKLET_scheduled, tasklet_work);
>     case 0:
>         /*tasklet_work_scheduled = 0;*/
>         break;
>
> Or is my code out of date?
>
> With or without that change,
> Reviewed-by: Don Koch <dkoch@verizon.com>

Another heuristic is a case statement followed by another case which
begins with a break.  The defect would re-emerge if code gets uncommented.

~Andrew