Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 9/22/21 9:39 AM, Jan Beulich wrote:
> On 22.09.2021 15:29, Boris Ostrovsky wrote:
>> On 9/22/21 6:17 AM, Jan Beulich wrote:
>>> @@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
>>>                     unsigned int i;
>>>  
>>>                     for (i = 0; i < num; i++) {
>>> -                           rc = pfns[i];
>>> +                           rc = errs[i];
>>>                             if (rc < 0)
>>>                                     break;
>>
>> Can the assignment be moved inside the 'if' statement?
> I wouldn't mind, albeit it's not the purpose of this change. Plus
> generally, when I do such elsewhere, I'm frequently told to better
> leave things as separate statements. IOW I'm a little surprised by
> the request.


Sure, can be done as a separate patch.


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>


>
>> I am also not sure I understand why we need error array at all. Don't we 
>> always look at the first error only? In fact, AFAICS this is the only place 
>> where we look at the value.
> Well, to look at the first error we need to scan the array to find
> one. Indeed we bail from here in once we've found a slot which has
> failed.
>
> I guess what you're trying to say is that there's room for
> improvement. In which case I might agree, but would want to point
> out that doing so would mean removing flexibility from the
> underlying function(s) (which may or may not be fine depending on
> what existing and future requirements there are).


We haven't needed this for a while and IMO existing code, with overloading the 
meaning of the pfn array, is rather confusing, unnecessarily complicated and 
error-prone (thus your patch).


>  And that would
> be for another day, if at all.


True.