Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2024 12:51, Federico Serafini wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>  
>      case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>          return arch_virq_is_global(virq);
> +
> +    default:
> +        ASSERT(virq < NR_VIRQS);
> +        break;
>      }
>  
> -    ASSERT(virq < NR_VIRQS);
>      return true;
>  }

Just for my understanding: The ASSERT() is moved so the "default" would
consist of more than just "break". Why is it that then the "return" isn't
moved, too?

> @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
>          break;
>      default:
>          ret = -EINVAL;
> +        break;
>      }

I certainly agree here.

> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>          case ECS_VIRQ:
>              printk(" v=%d", chn->u.virq);
>              break;
> +        default:
> +            /* Nothing to do in other cases. */
> +            break;
>          }

Yes this, just to mention it, while in line with what Misra demands is
pretty meaningless imo: The absence of "default" says exactly what the
comment now says. FTAOD - this is a comment towards the Misra guideline,
not so much towards the specific change here.

One other remark though, considering the specific function we're in: In
a certifiable environment, will there actually be the capability to
issue debug keys? Shouldn't we have a Kconfig option allowing to remove
all that from a build (and then also from relevant scans)?

Jan