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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
