Re: [Xen-devel] [PATCH V5] x86/altp2m: Hypercall to set altp2m view visibility

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 03.03.2020 11:48, Jan Beulich wrote:
> On 03.03.2020 10:43, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 03.03.2020 11:30, Jan Beulich wrote:
>>> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>>>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>>>>            break;
>>>>        }
>>>>    
>>>> +    case HVMOP_altp2m_set_visibility:
>>>> +    {
>>>> +        uint16_t idx = a.u.set_visibility.altp2m_idx;
>>>
>>> Why a fixed width type (and even one inefficient to deal with)?
>>> (One might even ask - why a local variable in the first place,
>>> when it's used ...
>>>
>>>> +        if ( a.u.set_visibility.pad )
>>>> +            rc = -EINVAL;
>>>> +        else if ( !altp2m_active(d) )
>>>> +            rc = -EOPNOTSUPP;
>>>> +        else
>>>> +            rc = p2m_set_altp2m_view_visibility(d, idx,
>>>> +                                                
>>>> a.u.set_visibility.visible);
>>>
>>> ... just once here.) The function takes "unsigned int" in any
>>> event.
>>
>> Sure, I can have this idx dropped and use the value in the structure.
>> I had that in place to have line size smaller and the code easy to read.
> 
> Dropping the variable is secondary - if you prefer you keep it, so
> be it. But if you keep it, its type should by in line with
> ./CODING_STYLE.
> 

Ah yes, you are right, I will change the type to unsigned int.


On the rc point you mentioned, I think it will be better to have a goto 
label there and have the cleanup on "out:"


Thanks for the review,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel