[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |