Received-SPF: pass (google.com: domain of linux-kernel+bounces-53798-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 5 Feb 2024 11:06:42 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Peter Zijlstra <peterz@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>, "Fabio M. De Francesco"
	<fabio.maria.de.francesco@linux.intel.com>, Jonathan Cameron
	<Jonathan.Cameron@huawei.com>
CC: Ingo Molnar <mingo@kernel.org>, Oleg Nesterov <oleg@redhat.com>,
	<linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>, Ira Weiny
	<ira.weiny@intel.com>
Subject: RE: [PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements
 in fail
Message-ID: <65c131c2e2ec6_4e7f529442@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240205-cond_guard-v1-1-b8d597a30cdd@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240205-cond_guard-v1-1-b8d597a30cdd@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L1ozYkE5cENFRVBjV2hYUWZXaDQvRk8xSy81YU9ydWppU0duMmVZRmFMcm5m?=
 =?utf-8?B?V2E1LzR5U2tIeGswV3YwenRUZDlxM0VCVVFFemx2cjY5YVdtMHYxeVZBSzhG?=
 =?utf-8?B?VUROYTA1bEZldlJZdjZVb042bFVCcXRGeDltYWJPakJxRk5qUEJva2JIZHJT?=
 =?utf-8?B?MzNjWXFBUDBXQ0pHblNNeS9XeXFUYktlZzBSSk8rajZQQ25kNnZyM3JMQ2k2?=
 =?utf-8?B?bk9oaFd4ZEJYa2xyZFlGTndpRmJESW44Z2J0bHpyckJzQmJrcVd1MndiWjdT?=
 =?utf-8?B?SjhqWjlkM2crZS92R1l6clhHd294V2IyR0NVVjhYRXkrUFExODRRNFlkQk51?=
 =?utf-8?B?ZVlIajVvZUdQT3poRkVNbUd1WWo2ejhIQTd1b1lHQ1M2NFJWMEtLejZEdTVC?=
 =?utf-8?B?eDNpaThjeEY0b0xIMHFiUjlRYW5iaVJvdEEreVI4LzdzN0xFWlE3MVJwNENt?=
 =?utf-8?B?QmRIaG41TFV1RlVkK1hmUm1WZXVMQUoxMmhXRG93SnJ4ellRb1FtNFk1eTUw?=
 =?utf-8?B?RzFySXhGZTlpbTVoRFJzc2VhYms0TWpoNW03SHZvWnNqT1FMUzRSS1dJMUxn?=
 =?utf-8?B?clJGN05mRVFEZ0R4SjVZNWoxMC91UWRmeEpVUWJZalhuN2w3RmxOdnN6QldL?=
 =?utf-8?B?dDFUdUdqaWhWdDloQlJhQjNCeS9GSVBHa05tcFFzenRQNDVja1IrckIvblQy?=
 =?utf-8?B?SHJwTUVMOG5vWnd6REk1VEFjMjlUdzhGd2ZzV1RYS29aZGNIOWdINXlBNCtY?=
 =?utf-8?B?WmFZdnkraTdpZmR0ZEloRHN3SDJtdTNReFhydmJ2VTZxNDNZcFg0ajRTNFRD?=
 =?utf-8?B?SDRzeGpJbUxBZ3hGcWtrdTdjUDZ5ZVJrV3d3bnlEcHZ5VTI0cHRhM3RhOGd4?=
 =?utf-8?B?NUVpQVNqM1hxaUhjbmFGYzhURHFOazR1ZEd0aWhSdjFZdGczQWVIRmtxT25r?=
 =?utf-8?B?S2lIelJ4ZmozZDhLYTg0dXA0NGc0WmFhamsvcnVsblZqNFRaODhSanZIYmpD?=
 =?utf-8?B?Ujk3VnJOMWQzUDhxeGJvcmpXbkxPZ2UzRHNleWJEVHpFbm1RSFlvZGxDTEZl?=
 =?utf-8?B?YUZHd3JBbmRoZERLWFFHQ3RnT01QYi9LVng4WmZjc1A0QlpNUDN1cWlEYlkz?=
 =?utf-8?B?SmJ5VTVEUzhDV1E1RVFVZi9rQ251R1VKZktrQ2xGRTg0ZEN4ZVdxd2QycFkz?=
 =?utf-8?B?NTR0Zk91WlBScmJKQmo4YkxXcVV2SGZJY3l2QmhnYVpnN3htdDFIYlZkbkxL?=
 =?utf-8?B?Q0NYd1oycDgwQ1RVTmRtVkxKSVFxUVJkVmo3TERCL0JyMVNWckszbERWUVQv?=
 =?utf-8?B?THZHOUQzRDRKN3dSZFlhK2NnY1RTa0FIcG9IUWxKUGZ3bVJ3cUtlTzBwTnZ0?=
 =?utf-8?B?M3JFd1QydnhDRnZydTZsUXYyMkpvbml5Uy9KTzZ0UTY2NHJWYitpb1hBa01J?=
 =?utf-8?B?dUZLd2I0ZW5Ra1VEOVFiam1OSFZ2RXU5dm5QdDJGYWhoYVZZTFd2SEpKaHZF?=
 =?utf-8?B?Uy9TRVVjeXd2M25MS2xoRG5vRXBzdm8rcTJaVWpSa2RFTzRPZ3pxTSttV3p5?=
 =?utf-8?B?WU1MeWV2aVRVRUpPb2FIc0VsaG5YRklQcWlFMmJZeG0xTzdrZU9ZRmROUTJy?=
 =?utf-8?B?Y0tWRG84T2pDdVBxV1Y4MzlmYjJhekoxeEx4Q3VBYlR5U1FRa1pqT1B5VEdj?=
 =?utf-8?B?WDBhcHc1c2hleWRzbWNNcC9xNEticGZhZTlPWmVmNFZpemdRaU5pU1hNNUlU?=
 =?utf-8?B?amw3ZnN5OGhmc1RmSjM2NnI0OVZVWTA3ZElYdjhVUDBNbGpIVnpBeVAvU3Bo?=
 =?utf-8?B?ZW1jcU5oVE1mTEhLYzRLekdZTU10TzkvRVNWejEweW1sc25RZG9icGpWUHZS?=
 =?utf-8?B?OGQwcXN2cWFyZVQxSjhBTXJxaFQrRkdsZlJLbGFPMW4zMGZlT1RNVXdqYTRz?=
 =?utf-8?B?b2xJUVZQeHNvMmUrYkIwYllkcTJCYWhIKy9nV09hL2poTHdLYVpvZWZjZlQ3?=
 =?utf-8?B?aUVONllZQkYySnJVZVp0Nm5TcXhYOFpTbFQ1L2VuNDNvV3FJZE9xd1h1T0xP?=
 =?utf-8?B?VnpyeW9VaEVOZXBhOWNsOElFbm5oOEp3YnJxWEpXMjFFQ0NrQlh1cElPV0VG?=
 =?utf-8?B?a3dkd3plSXJyRDdRYmc2ZkYxN1FDM2VTL3MvWWRSVEVsY0cySWMwdmVteXBF?=
 =?utf-8?B?Y1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dd164d0a-7313-4d19-71f4-08dc267d9866
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 19:06:45.0552
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: psEfhRxdMJNBKp5VCPIDHMvTxuVCiHeSig2Ng7+TzzQT9EwyfZzFuSY+8ArENjqKhG+z/4UG8ZGePlo7A1HkKNBWmKYfg3PNoCp2D4SB/3U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6958
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> In attempting to create a cond_guard() macro[1] Fabio asked me to do
> some testing of the macros he was creating.  The model for this macro
> was scoped_cond_guard() and the ability to declare a block for the error
> path.
> 
> A simple test for scoped_cond_guard() was created to learn how it
> worked and to model cond_guard() after it.  Specifically compound
> statements were tested as suggested to be used in cond_guard().[2]
> 
> static int test_scoped_cond_guard(void)
> {
>         scoped_cond_guard(rwsem_write_try,
>                         { printk(KERN_DEBUG "Failed\n"); return -EINVAL; },
>                         &my_sem) {
>                 printk(KERN_DEBUG "Protected\n");
>         }
>         return 0;
> }
> 
> This test fails with the current code:
> 
> lib/test-cleanup.c: In function ‘test_scoped_cond_guard’:
> ./include/linux/cleanup.h:190:17: error: ‘else’ without a previous ‘if’
>   190 |                 else
>       |                 ^~~~
> lib/test-cleanup.c:79:9: note: in expansion of macro ‘scoped_cond_guard’
>    79 |         scoped_cond_guard(rwsem_write_try,
>       |         ^~~~~~~~~~~~~~~~~
> 
> This is due to an extra statement between the if and else blocks created
> by the ';' in the macro.

A statement-expression "({ })" builds for me, did you test that?

> 
> Ensure the if block is delineated properly for the use of compound
> statements within the macro.
> 
> [1] https://lore.kernel.org/all/20240204173105.935612-1-fabio.maria.de.francesco@linux.intel.com/
> [2] https://lore.kernel.org/all/65b938c1ad435_5cc6f294eb@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> NOTE: There is no user of this syntax yet but this is the way that Dan
> and I thought the macro worked.  An alternate syntax would be to require
> termination of the statement (ie use ';') in the use of the macro; see
> below.  But this change seemed better as the compiler should drop the
> extra statements created and allows for a bit more flexibility in the
> use of the macro.
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 88af56600325..6cc4bfe61bc7 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -186,7 +186,7 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  #define scoped_cond_guard(_name, _fail, args...) \
>         for (CLASS(_name, scope)(args), \
>              *done = NULL; !done; done = (void *)1) \
> -               if (!__guard_ptr(_name)(&scope)) _fail; \
> +               if (!__guard_ptr(_name)(&scope)) _fail \
>                 else
> 
>  /*
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 2fabd497d659..fae110e8b89f 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -441,7 +441,7 @@ static int ptrace_attach(struct task_struct *task, long request,
>          * SUID, SGID and LSM creds get determined differently
>          * under ptrace.
>          */
> -       scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> +       scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR;,

..otherwise, that semicolon looks out of place and unnecessary.

>                            &task->signal->cred_guard_mutex) {
> 
>                 scoped_guard (task_lock, task) {
> ---
>  include/linux/cleanup.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 88af56600325..d45452ce6222 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -186,7 +186,7 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  #define scoped_cond_guard(_name, _fail, args...) \
>  	for (CLASS(_name, scope)(args), \
>  	     *done = NULL; !done; done = (void *)1) \
> -		if (!__guard_ptr(_name)(&scope)) _fail; \
> +		if (!__guard_ptr(_name)(&scope)) { _fail; } \
>  		else
>  
>  /*

Why 2 changes to include/linux/cleanup.h in the same patch?