[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <65c131c2e2ec6_4e7f529442@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 5 Feb 2024 11:06:42 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Peter Zijlstra <peterz@...radead.org>,
Dan Williams <dan.j.williams@...el.com>, "Fabio M. De Francesco"
<fabio.maria.de.francesco@...ux.intel.com>, Jonathan Cameron
<Jonathan.Cameron@...wei.com>
CC: Ingo Molnar <mingo@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Ira Weiny
<ira.weiny@...el.com>
Subject: RE: [PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements
in fail
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@...el.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?
Powered by blists - more mailing lists