| Message ID | 20240205-cond_guard-v1-1-b8d597a30cdd@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] cleanup/scoped_cond_guard: Fix multiple statements in fail | expand |
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?
Dan Williams wrote: > 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? I did not test that, no. Would that be the syntax expected? I'm not familiar with any macros like this so perhaps I misunderstood the expected use. > > > > > 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. Agreed. This is an alternative I considered but rejected and was only mentioning it as part of the commit message. Sorry for the confusion on this. This is not the patch suggested. See below. > > > &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? Sorry for the confusion. This is the patch itself and my suggested fix. It does not require any changes to kernel/ptrace.c The diff above was just an alternative I thought about. Ira
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;, &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 /*
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. 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. --- base-commit: 03c972291873663f15c78ff4ca07cbf5025735f8 change-id: 20240201-cond_guard-afa0566cecdf Best regards,