Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource
 processing
To:     Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc:     Stefano Stabellini <sstabellini@kernel.org>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Juergen Gross <jgross@suse.com>
References: <0f0db6fa-2604-9a0d-1138-0063b5a39a87@suse.com>
 <aa6d6a67-6889-338a-a910-51e889f792d5@suse.com>
 <c0c84258-c2ee-f58c-ae9a-5f8bdd75f0db@oracle.com>
From:   Jan Beulich <jbeulich@suse.com>
Message-ID: <1374b8da-1076-63fb-bc54-5be9f1ae94d4@suse.com>
Date:   Wed, 22 Sep 2021 15:39:29 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <c0c84258-c2ee-f58c-ae9a-5f8bdd75f0db@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NXpZcmY5dWZRZEVkY3cxaWFlSWxpd2FaNkh0NE52MjdXQzZlaFF5Tmk1cDRM?=
 =?utf-8?B?NDAvUTZ6NGhYZS9YV2lQS2pvTjVGWWJVWFMrb0tjRENQQlh1VUd6YWgrbmwz?=
 =?utf-8?B?c0JZcXg0citQYW1Lc3BwclgxVXZVd1REZWFsVHovK2h3M3ppMi9BR2dVUTZt?=
 =?utf-8?B?YU9RNDVzWktGRFMxcGZMMjZxWVZwdVVId1JCVEw2ZjJNclhDUE5wemlubzEw?=
 =?utf-8?B?RnVZTEZVdTUwMERyQU9mMFl1bG13VjNINCtSUmprVlhJdDNSdVpTVGRtVndw?=
 =?utf-8?B?aU40V3Nrc2traUVEVGR6dENnV085WnBIWVl1aC9nOWlUOU1XR1ZKNXduMFdW?=
 =?utf-8?B?N1ZRRTVadGEyblFEMG5mdTZRYzh0SkdJL3RYSk5KeVNoMXhzUHNnTXFGVTFZ?=
 =?utf-8?B?TFRoZjEzK3ZPTWdMUWFxcmUyTzBITDFTSlp5c2ZTVC9tRVZYMHIvQlgwMkx3?=
 =?utf-8?B?NUtzQTJWMzEyMmtCTUUva1RuYlNXSG45cWszR1V5OEIzQ3prV204ZFhOTEJQ?=
 =?utf-8?B?bWlEMWVNQkVaem84WGtHVEtndjBhWDZPK3VFNXBTc0MzZkdhTnVsaUdFdndS?=
 =?utf-8?B?aUUxUnBXUGRTa1V5M1JnbmZoQUdPLzFIaGpYSjNVSGJpS0pNODRLbjNMUEpk?=
 =?utf-8?B?UDV6elp3WGFKTE52SURVT2xoMGRFWU9PWkp0azRYSDdWUjRuVXlFYUJZTlFs?=
 =?utf-8?B?d0E0RzFjcXUrSDFRckN4SGRlKzlXWkNrZUZtWEJPdlZ6SWxvVzk3OFlhVkVN?=
 =?utf-8?B?SzFLRUJUY28yR2cvdlY0L3lSRElqSWZuMGdqSlBVUTN6MUdtU0NyTmgrSmw5?=
 =?utf-8?B?Ui8zTmhzU3hrcVlxOEw0ZS83SmZMZWw4WGJWRW55Y2tGd0dXWFY5VCtMbmpK?=
 =?utf-8?B?MXVzZ0tZcEZURjZkWFhtOUZYVHJDVEhiWWZNUlpYZE5JRCtabmFaa00yblZ5?=
 =?utf-8?B?V0NVQmdIN3hSUmJUaDRLSC94RTlMcFNxeDVpWFV3RFNtZmp2RXhsVFZYeUJx?=
 =?utf-8?B?akVRSSsrS3BpdWowK2NXeFBlcmFFUmFTS1RlaGM1ekNEZXNsVDJ2ZWxIRnM5?=
 =?utf-8?B?aVNZNWxFbkNhQXpnTXlSSzRjazBkL3NIQVAwUUdkWFFZaUI1bENIb21DTkxX?=
 =?utf-8?B?ZGRaV0hLREpPU0xPYmtiRjBlaEtFTTlPRjJtY0Z1cXFHSEJWcHFLZzZyUmZ2?=
 =?utf-8?B?cTFJQndJbmNzQTlUTmFCQTZYVTlZVnJlYVdXNzRINWZPdjdJSVBlT3lmbXZN?=
 =?utf-8?B?eE42bW1mVzJ1MVJGdkNCSmd2ZFhUUnc5d2NKTHdYeWcwRFNCQ0xVenJaQkIv?=
 =?utf-8?B?OWw1aXhWVUdCSHRXekh4UHJKMENreVRqUlpEd1R3MVdKbGxPM2ZYdXpFaEZK?=
 =?utf-8?B?djJPU2dNZ1AvUDVBVVhWR3I0UTE0NGVvWmpGNDRTaHJpSG9jV1E3UGNmdmdt?=
 =?utf-8?B?clpVTlU4RnFpUC9BVSt3UkhobzFkK01RUFBFOVhNRXFqR01Nc3FoeFNWdkl1?=
 =?utf-8?B?dktGWGpKS01abFN0QXc3ZVZ5NWR5ck8xYm40MEE5ZlR0dmExb3RMVUU2WFRt?=
 =?utf-8?B?WDVOcFB5cDdVdzdSTDN2N2dxV1dqdnlhOGVYazNqR2VwYjkrZmZiZGRzS3BF?=
 =?utf-8?B?ZndEMnVRYUJjT1V4UGQrME4rd2ppZXlvQjk3aVJCVWxiUS9remdRMDJ0V0hM?=
 =?utf-8?B?ejhlZis1NjBUWG5jT1hFMjFhSHp0SXhNT0luSkJHdzNZTXlLc2h2RklIaFdX?=
 =?utf-8?Q?ndu5HqEnxOyzRixjLNhnodr1+MJkYeq+EbQm1aZ?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dc48c9ae-95b2-4e4b-271b-08d97dce6755
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5600.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2021 13:39:30.1343
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VVxl4hIccXeUj0uZ5RptOOBqE1jl9EMKz2Ejxf/yZtYltdG5VGzD0kQF4Kdx3Ihn59eiKSVC6LuwRyhuy/korg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB7039
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

> 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). And that would
be for another day, if at all.

Jan