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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2020 10:59, Alexandru Stefan ISAILA wrote:
> 
> 
> 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:"

My general position is that when error paths are complicated, "goto"
is acceptable. But when things can easily be done without "goto", its
use would better be avoided. In the case here all you need (afaict)
is a simple sequence of if/else-if/else.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel