Re: [PATCH 1/1] target/arm: Adding a check for the result of calling the CPU information check function

[Alex Bennée]

Sergey Mironov <mironov@fintech.ru> writes:

> 6 out of 7 calls to get_arm_cp_reginfo() are checked

Yes but we should be careful with asserts (vs if (ri) legs) because I
don't think get_arm_cp_reginfo() guarantees it will always be
successful.

>
> Signed-off-by: Sergey Mironov <mironov@fintech.ru>
> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 74fbb6e1d7..cffbbaf571 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -198,6 +198,7 @@ static void add_cpreg_to_list(gpointer key, gpointer 
> opaque)
>      uint32_t regidx = (uintptr_t)key;
>      const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, regidx);
>  
> +    assert(ri != NULL);

  /* must always succeed as we are iterating the keys of cp_regs */
  assert(ri);

is enough for a !NULL check.

>      if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_ALIAS))) {
>          cpu->cpreg_indexes[cpu->cpreg_array_len] = cpreg_to_kvm_id(regidx);
>          /* The value array need not be initialized at this point */

That said we already have an assert that would fire in
init_cpregs_list():

  assert(cpu->cpreg_array_len == arraylen);

so I'm not sure what this is adding to ensuring the contract is kept.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro