Re: [Xen-devel] [PATCH v12 15/23] x86: refactor psr: CDP: implement set value callback function.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 10:40, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-07-03 01:01:09, Jan Beulich wrote:
>> >>> On 03.07.17 at 08:33, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-06-30 06:02:32, Jan Beulich wrote:
>> >> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/30/17 1:30 PM >>>
>> >> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] 
>> >> >is 
> CODE.
>> >> >In the first iteration, the props->type[0] is DATA so that it does not 
>> >> >match
>> >> >'type' and the second check is false too. If we use else branch here, it 
> will
>> >> >enter the branch and return -EVINVAL. But this is not we want, right? We 
> hope
>> >> >the second iteration should be executed to set CODE.
>> >>  
>> >> I see. That'll then call for yet another solution; I don't think the code 
> should
>> >> stay as is.
>> >> 
>> > Then, how about ASSERT() at the beginning to check if input 'type' is 
>> > correct?
>> >     enum cbm_type {
>> >         PSR_CBM_TYPE_L3,
>> >         PSR_CBM_TYPE_L3_DATA,
>> >         PSR_CBM_TYPE_L3_CODE,
>> >         PSR_CBM_TYPE_L2,
>> >     };
>> > 
>> >     ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 
>> > 1]) ||
>> >            type == props->alt_type);
>> 
>> Baking in ordering assumptions? No, please don't.
>> 
>> > We don't need 'ret' anymore with above check.
>> 
>> So in a release build you'd then do what in case of a bad type finding
>> its way in?
>> 
>> Jan
> 
> To decide the return value, we have to know if input 'type' is correct or 
> not.
> There are two ways:
> 1. Check if input 'type' without iteration, like the above codes. Becaue you
>    don't agree the ordering assumptions, this way is not good.
> 2. Use iteration, like the original codes. Record if the statement is hit.
>    If yes, return 0. Otherwise, return -EINVAL. The original codes are below:
>     for ( i = 0; i < props->cos_num; i++ )
>     {
>         if ( type == props->type[i] || type == props->alt_type )
>         {
>             val[i] = new_val;
>             ret = 0;
>         }
>     }
> 
> I think the main issue you don't like in the original codes is that
> the 'ret = 0' may happen for several times. How about below change?
>     for ( i = 0; i < props->cos_num; i++ )
>     {
>         if ( type == props->type[i] || type == props->alt_type )
>         {
>             val[i] = new_val;
>             if ( ret )
>                 ret = 0;
>         }
>     }

No, the multiple assignments would be no issue at all. As said
before, what I dislike is the wrongness of the return value if
the first iteration sets ret to zero, but a subsequent one
wouldn't. In that case, an error should be signaled.

Jan


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