Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2023 17:00, Julien Grall wrote:
> 
> 
> On 23/10/2023 15:51, Nicola Vetrini wrote:
>> Hi,
> 
> Hi Nicola,
> 
>> while taking care of some patches regarding MISRA C Rule 2.1 (code 
>> shouldn't be unreachable), I
>> came across this function:
>>
>> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> {
>>      switch ( domctl->cmd )
>>      {
>>      case XEN_DOMCTL_set_address_size:
>>          switch ( domctl->u.address_size.size )
>>          {
>>          case 32:
>>              if ( !cpu_has_el1_32 )
>>                  return -EINVAL;
>>              /* SVE is not supported for 32 bit domain */
>>              if ( is_sve_domain(d) )
>>                  return -EINVAL;
>>              return switch_mode(d, DOMAIN_32BIT);
>>          case 64:
>>              return switch_mode(d, DOMAIN_64BIT);
>>          default:
>>              return -EINVAL;
>>          }
>>          break;
>>
>>      default:
>>          return -ENOSYS;
>>      }
>> }
>>
>> here the break after the innermost switch is clearly unreachable, but 
>> it's also guarding a possible fallthrough.
>> I can see a couple of solutions to this:
>>
>> - mark the part after the switch unreachable;
>> - introduce a variable 'long rc' to store the return value, and 
>> consequently rework the control flow of all the switches
>>    (e.g. rc = -EINVAL and similar);
>> - remove the break, but I consider this a risky move, unless -ENOSYS 
>> would be an ok value to be returned if some case
>>    from the switch above does not have a return statement.
> 
> - move the nested switch in a separate function, so the code in 
> subarch_do_domctl() can be replaced with:
> 
> return set_address_size(...);

But that would help only if inside the new function you still re-
layout the switch() (or replace it by, say, if/else-if/else),
wouldn't it?

Jan