Re: [Xen-devel] [PATCH RESEND v5 09/24] x86: refactor psr: set value: assemble features value array.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

>  static int assemble_val_array(uint64_t *val,
> @@ -550,7 +641,25 @@ static int assemble_val_array(uint64_t *val,
>                                const struct psr_socket_info *info,
>                                unsigned int old_cos)
>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    int ret;
> +    uint64_t *val_tmp = val;
> +
> +    if ( !val )
> +        return -EINVAL;
> +
> +    /* Get all features current values according to old_cos. */
> +    list_for_each_entry(feat, &info->feat_list, list)
> +    {
> +        /* value getting order is same as feature list */
> +        ret = feat->ops.get_old_val(val_tmp, feat, old_cos);

Shouldn't we check ret for negative values?
> +
> +        val_tmp += feat->ops.get_cos_num(feat);
> +        if ( val_tmp - val > array_len)
> +            return -EINVAL;


Perhaps: ENOSPC ?

Also this function does do any assembling. It just verifies.
Perhaps other patches add extra work here? In which case you may
want to mention that in the commit description.

> +    }
> +
> +    return 0;
>  }
>  
>  static int set_new_val_to_array(uint64_t *val,
> @@ -560,7 +669,37 @@ static int set_new_val_to_array(uint64_t *val,
>                                  enum cbm_type type,
>                                  uint64_t m)
>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    int ret;
> +    uint64_t *val_tmp = val;
> +
> +    /* Set new value into array according to feature's position in array. */
> +    list_for_each_entry(feat, &info->feat_list, list)
> +    {
> +        if ( feat->feature != feat_type )
> +        {
> +            val_tmp += feat->ops.get_cos_num(feat);
> +            if ( val_tmp - val > array_len)
> +                return -EINVAL;

-ENOSPC?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel