[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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
  • Date: Wed, 22 Sep 2021 09:56:53 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=zhxTcPI7rWwl4C7tspU9gg/f3idfnYkP5xBPg20EQqo=; b=J3Y8eaDvOs18hD3QkcM+iQ2W2a0nEAwMM+KhYFZXAzu/UWhx2xWDDsiklZZVDZVzhU3mZpBGDxOvx0WQvs32dC0Q6mYrcDmzpHMCJr0Vs9weMDyXejJ3AcBJ1DgdomUQ0+SuD7MTmrpMOqVBkPcqd+0uCJjuqKndmag1aGUoEIwgetVigej8aRlm3s4aAiAaUSomelX0EkUE6oxbI/sMYsn840XVjjgZzRVlbGEIkfaPdDaroFoxAfxQnXiykWSS+P6WT8t7r2YhGScc3aK7qhRoMzwdyR51+pHHYjytEMNM3qScbSt6O+Ybz8Gb/N9g6kTPSOireds93i3IjED9QA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=hRKiGgoqEdwvGVd2BNVNylRTzfy7dPF14q9Vcak/EKcjiBkfoGpyEIMuo6Svve1q4r+87us9bq9jXvTTtJ7doDf5Ir95XxMNA9OFvCjNMqaqYo8Hi10UebS5bKYcW6ahcBoyRcwog5mrLSg9i+yJEcdz/6KGv3DxfE9Vf42/aeh+epvz+0qnn+c3gpoAfA21yHt7tnRGY9/krqPPA8pYAyHTAvhzCQyfMiScaiS0hAxxf8x+ZFKVD0CAH8rVRqjQI3t/3X9pRLpc/oVi39HAB3ZaxCV1IPkoCSjbE43pjBq7TzwrI6K4QhxRPR3nOGDAnavwXN4KbPrfrQaJUK5LoQ==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=oracle.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, lkml <linux-kernel@xxxxxxxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
  • Delivery-date: Wed, 22 Sep 2021 13:57:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.