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:     Jan Beulich <jbeulich@suse.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>
 <1374b8da-1076-63fb-bc54-5be9f1ae94d4@suse.com>
From:   Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <ccf710f1-3c2f-880c-fa89-64fabe852527@oracle.com>
Date:   Wed, 22 Sep 2021 09:56:53 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
In-Reply-To: <1374b8da-1076-63fb-bc54-5be9f1ae94d4@suse.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d1VOT01ySklBWTlKbDZKaTZsTkhObjFTakVmV05YRTlWY2FqSGw1ZTRyeTZ4?=
 =?utf-8?B?OFgzeEZ4azZUM0kzV2lUSFREVGJ3clFMN3dtTmZjWTBvNjU2djgrTjlKREc4?=
 =?utf-8?B?cUE4eXR2SlN6WUI4L2lBN2MyenJvNUVtV1RSdmRudk5seUoweTdPcXk1TjFj?=
 =?utf-8?B?c0Y4aHV1QWFqYVJqN3ZwR3A4dTUzWHpycVY2cDlUSkpJY2U1amt6TjVjSnNP?=
 =?utf-8?B?ckZJSDliQVFlTlliaXdtU2V5WExvOXpPK0E5Qkd5aDFTN1hKZEVrYlNSNWJn?=
 =?utf-8?B?eXNPeEtQWXJFdUpMbm5GL2ZkMzNVS2xVV3NKUXJWanowaEt0emJyczJwZVVw?=
 =?utf-8?B?L1duTDI5c0lmMysyUnlVYkh3VlYxYTFUNWtWam9LcnRjaTh4Nk4vekQ4ZHZv?=
 =?utf-8?B?c2ZKZms1NFB0L0w2WGRQeUcrSCtvSEoyQzZmd1hic091ZWpUMmg5S0RCaTNQ?=
 =?utf-8?B?bHJmbm00NWFSN1RVWFpBeFRIRlZPOFB5bk1kTWUxYkc5V1JaV3A1b29zOFJw?=
 =?utf-8?B?Y3lzNGNHZ1NrcTJNSCtidTdJSWc4Y3B1dUljM3FPYzgyZXhpdFZ1TTgvM2kv?=
 =?utf-8?B?Sk04aXd1SVA3MTRXTUdlQ1dvK0s1ejlyTEdMaE9EUG5IaU9ienVIakwxUW4w?=
 =?utf-8?B?NWo3Q1NLaVJTK2d1aVJGcjVFVEZkbDhMUW9HM0ZkbHY3ZVFqZVlaQ05zNUJ4?=
 =?utf-8?B?eDRQSCtNUVVDc3ZzcEFxdWFjc08xRm81RjRHNVlsaTNZVDN6eGt5OUc0OGc3?=
 =?utf-8?B?UlRnNFdvYzJHU0RXUHJUREVjV3lIUTN3QkJBNkxzRzB4OXVpZzkxTGdzL3pv?=
 =?utf-8?B?TzZuODdTVVdjaG1HUmNJNWd6N2hUazdjRzBTSUJlamFodlE3bFFFcmlCZFR6?=
 =?utf-8?B?ak5VNTQ0N01OcWkxb0xLNDBkbkpLeVdwREU1ZUNzTlV4MWxibExneWVrNVh0?=
 =?utf-8?B?bm9WcWo5MHljUjVZZTdHVWc3Q3lTaXNCVzRrNVMrcVk0a045OXZQZDlzNkk3?=
 =?utf-8?B?MEl3K1BGNEhUWWVKVjRSc3JPVlIzUkxkUTEzZE1WcFVsOGRTeFFlVWk0dyt5?=
 =?utf-8?B?MkZqREYyUm9wMFV1V2dOaUxsT0JWRVBUMnRmaW95ZmNwRG1TVVg3WTVzWFF4?=
 =?utf-8?B?RDZIdkxkMGxyWDh5TTVzV3FzeGo3YUlBMVZpVnRLTytRU2pTYTE2cjRLc1Ux?=
 =?utf-8?B?MWtObE9HQ3BqZ3NvQnlUbU9LbEJiZDliNHlnKzJ2a3JNRFBDWTFlcnN5SUZP?=
 =?utf-8?B?TVU1RVVEcytCTDZlV3VpN0dHcnA5YnRiRnEzdXpXMHp3Y3orcGpJTG9mYVYv?=
 =?utf-8?B?T3Q2MFIxWGo0UkJkLzhpbjdtNWVtVXNFK05zNTIrZXJaWWV2VHBnUXRNaUQ0?=
 =?utf-8?B?K0ZhM2Y5YXd5NG5Fcm1ya1A0T01EbHZ1bzlhMThkVWs3alVzejYyWWJrZGtX?=
 =?utf-8?B?dk5CSU5KZUZ5MTFQYnl3cFA0VEc0TlU2MmIrWnFPcEx6OU9rSXppQmdQUDNF?=
 =?utf-8?B?THBKRVk1SVVoS3A2SlZzSjhIaTM2cHhqOEFiOHVzU01QaU5QeWZVemRQcDlQ?=
 =?utf-8?B?bkJkeVpKcldYVUV4d3JrSXdocUFHdFlKYWF2eE5zS3RLQXRwMTRMTTQ0R1Ey?=
 =?utf-8?B?V201N2RMVjNxMTVmeHhBWTJhVG5kV0twdjhHSlN5ZTRBSGprMFZDRm52T3c5?=
 =?utf-8?B?YW11QlM0eDZnNHRadnpEL1dUcXZsKzh5dElYQjFTUDhlTWNpY3BQZWIzUk43?=
 =?utf-8?Q?Ip+vuNVCU+4G3SWAChuLr0QOw18JHessWC+crAn?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ab6b2368-65e3-4b2c-2fcc-08d97dd0d7cb
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB5009.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2021 13:56:57.8116
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: adL9GHjmxFK8opL1cf68tCzx5vX3IF2TAjQDLeWSM3zkt5e+R2FV6p2OjI1FchAzgqZXyAI5A96Y5eGQq+H5PMuvxzhzBtsiUGfmegMAhpk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4109
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10114 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999
 spamscore=0 malwarescore=0 adultscore=0 mlxscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109200000 definitions=main-2109220100
X-Proofpoint-ORIG-GUID: 4gUOny7eEi5k4lSyVMf7k2yU2bhGGkBU
X-Proofpoint-GUID: 4gUOny7eEi5k4lSyVMf7k2yU2bhGGkBU
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.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@oracle.com>


>
>> 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.