| Message ID | 20240509200022.253089-55-edliaw@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Define _GNU_SOURCE for sources using | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On 2024-05-09 15:58, Edward Liaw wrote: > _GNU_SOURCE is provided by lib.mk, so it should be dropped to prevent > redefinition warnings. > > Fixes: 809216233555 ("selftests/harness: remove use of LINE_MAX") The patch per se looks fine, except for the "Fixes" tag. Commit 809216233555 introduces use of asprintf in kselftest_harness.h which is used by (all ?) selftests, including the rseq ones. However, the rseq selftests each have the #define _GNU_SOURCE, which would have been OK without those further changes. So this patch is more about consolidating where the _GNU_SOURCE is defined, which is OK with me, but not so much about "fixing" an issue with commit 809216233555. A "Fix" is something to be backported to stable kernels, and I don't think this patch reaches that threshold. If anything, this patch removes a warning that gets added by https://lore.kernel.org/lkml/20240509200022.253089-1-edliaw@google.com/T/#mf8438d03de6e2b613da4f86d4f60c5fe1c5f8483 within the same series. Arguably, each #define _GNU_SOURCE could have been first protected by a #ifndef guard to eliminate this transient warning, and there would be nothing to "fix" in this consolidation series. Thoughts ? Thanks, Mathieu > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Signed-off-by: Edward Liaw <edliaw@google.com> > --- > tools/testing/selftests/rseq/basic_percpu_ops_test.c | 1 - > tools/testing/selftests/rseq/basic_test.c | 2 -- > tools/testing/selftests/rseq/param_test.c | 1 - > tools/testing/selftests/rseq/rseq.c | 2 -- > 4 files changed, 6 deletions(-) > > diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c > index 2348d2c20d0a..5961c24ee1ae 100644 > --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c > +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c > @@ -1,5 +1,4 @@ > // SPDX-License-Identifier: LGPL-2.1 > -#define _GNU_SOURCE > #include <assert.h> > #include <pthread.h> > #include <sched.h> > diff --git a/tools/testing/selftests/rseq/basic_test.c b/tools/testing/selftests/rseq/basic_test.c > index 295eea16466f..1fed749b4bd7 100644 > --- a/tools/testing/selftests/rseq/basic_test.c > +++ b/tools/testing/selftests/rseq/basic_test.c > @@ -2,8 +2,6 @@ > /* > * Basic test coverage for critical regions and rseq_current_cpu(). > */ > - > -#define _GNU_SOURCE > #include <assert.h> > #include <sched.h> > #include <signal.h> > diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c > index 2f37961240ca..48a55d94eb72 100644 > --- a/tools/testing/selftests/rseq/param_test.c > +++ b/tools/testing/selftests/rseq/param_test.c > @@ -1,5 +1,4 @@ > // SPDX-License-Identifier: LGPL-2.1 > -#define _GNU_SOURCE > #include <assert.h> > #include <linux/membarrier.h> > #include <pthread.h> > diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c > index 96e812bdf8a4..88602889414c 100644 > --- a/tools/testing/selftests/rseq/rseq.c > +++ b/tools/testing/selftests/rseq/rseq.c > @@ -14,8 +14,6 @@ > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > * Lesser General Public License for more details. > */ > - > -#define _GNU_SOURCE > #include <errno.h> > #include <sched.h> > #include <stdio.h>
On Thu, May 9, 2024 at 1:16 PM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > On 2024-05-09 15:58, Edward Liaw wrote: > > _GNU_SOURCE is provided by lib.mk, so it should be dropped to prevent > > redefinition warnings. > > > > Fixes: 809216233555 ("selftests/harness: remove use of LINE_MAX") > > The patch per se looks fine, except for the "Fixes" tag. > > Commit 809216233555 introduces use of asprintf in kselftest_harness.h > which is used by (all ?) selftests, including the rseq ones. However, > the rseq selftests each have the #define _GNU_SOURCE, which would have > been OK without those further changes. > > So this patch is more about consolidating where the _GNU_SOURCE is > defined, which is OK with me, but not so much about "fixing" an > issue with commit 809216233555. > > A "Fix" is something to be backported to stable kernels, and I > don't think this patch reaches that threshold. > > If anything, this patch removes a warning that gets added by > https://lore.kernel.org/lkml/20240509200022.253089-1-edliaw@google.com/T/#mf8438d03de6e2b613da4f86d4f60c5fe1c5f8483 > within the same series. > > Arguably, each #define _GNU_SOURCE could have been first protected > by a #ifndef guard to eliminate this transient warning, and there > would be nothing to "fix" in this consolidation series. That makes sense. I can remove the fixes tags. 809216233555 will likely be reverted first anyway, and you're right that the focus of this patch series is on consolidating _GNU_SOURCE. > > Thoughts ? > > Thanks, > > Mathieu > > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > Signed-off-by: Edward Liaw <edliaw@google.com> > > --- > > tools/testing/selftests/rseq/basic_percpu_ops_test.c | 1 - > > tools/testing/selftests/rseq/basic_test.c | 2 -- > > tools/testing/selftests/rseq/param_test.c | 1 - > > tools/testing/selftests/rseq/rseq.c | 2 -- > > 4 files changed, 6 deletions(-) > > > > diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c > > index 2348d2c20d0a..5961c24ee1ae 100644 > > --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c > > +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c > > @@ -1,5 +1,4 @@ > > // SPDX-License-Identifier: LGPL-2.1 > > -#define _GNU_SOURCE > > #include <assert.h> > > #include <pthread.h> > > #include <sched.h> > > diff --git a/tools/testing/selftests/rseq/basic_test.c b/tools/testing/selftests/rseq/basic_test.c > > index 295eea16466f..1fed749b4bd7 100644 > > --- a/tools/testing/selftests/rseq/basic_test.c > > +++ b/tools/testing/selftests/rseq/basic_test.c > > @@ -2,8 +2,6 @@ > > /* > > * Basic test coverage for critical regions and rseq_current_cpu(). > > */ > > - > > -#define _GNU_SOURCE > > #include <assert.h> > > #include <sched.h> > > #include <signal.h> > > diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c > > index 2f37961240ca..48a55d94eb72 100644 > > --- a/tools/testing/selftests/rseq/param_test.c > > +++ b/tools/testing/selftests/rseq/param_test.c > > @@ -1,5 +1,4 @@ > > // SPDX-License-Identifier: LGPL-2.1 > > -#define _GNU_SOURCE > > #include <assert.h> > > #include <linux/membarrier.h> > > #include <pthread.h> > > diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c > > index 96e812bdf8a4..88602889414c 100644 > > --- a/tools/testing/selftests/rseq/rseq.c > > +++ b/tools/testing/selftests/rseq/rseq.c > > @@ -14,8 +14,6 @@ > > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > * Lesser General Public License for more details. > > */ > > - > > -#define _GNU_SOURCE > > #include <errno.h> > > #include <sched.h> > > #include <stdio.h> > > -- > Mathieu Desnoyers > EfficiOS Inc. > https://www.efficios.com >
diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c index 2348d2c20d0a..5961c24ee1ae 100644 --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c @@ -1,5 +1,4 @@ // SPDX-License-Identifier: LGPL-2.1 -#define _GNU_SOURCE #include <assert.h> #include <pthread.h> #include <sched.h> diff --git a/tools/testing/selftests/rseq/basic_test.c b/tools/testing/selftests/rseq/basic_test.c index 295eea16466f..1fed749b4bd7 100644 --- a/tools/testing/selftests/rseq/basic_test.c +++ b/tools/testing/selftests/rseq/basic_test.c @@ -2,8 +2,6 @@ /* * Basic test coverage for critical regions and rseq_current_cpu(). */ - -#define _GNU_SOURCE #include <assert.h> #include <sched.h> #include <signal.h> diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c index 2f37961240ca..48a55d94eb72 100644 --- a/tools/testing/selftests/rseq/param_test.c +++ b/tools/testing/selftests/rseq/param_test.c @@ -1,5 +1,4 @@ // SPDX-License-Identifier: LGPL-2.1 -#define _GNU_SOURCE #include <assert.h> #include <linux/membarrier.h> #include <pthread.h> diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c index 96e812bdf8a4..88602889414c 100644 --- a/tools/testing/selftests/rseq/rseq.c +++ b/tools/testing/selftests/rseq/rseq.c @@ -14,8 +14,6 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. */ - -#define _GNU_SOURCE #include <errno.h> #include <sched.h> #include <stdio.h>
_GNU_SOURCE is provided by lib.mk, so it should be dropped to prevent redefinition warnings. Fixes: 809216233555 ("selftests/harness: remove use of LINE_MAX") Reviewed-by: John Hubbard <jhubbard@nvidia.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Signed-off-by: Edward Liaw <edliaw@google.com> --- tools/testing/selftests/rseq/basic_percpu_ops_test.c | 1 - tools/testing/selftests/rseq/basic_test.c | 2 -- tools/testing/selftests/rseq/param_test.c | 1 - tools/testing/selftests/rseq/rseq.c | 2 -- 4 files changed, 6 deletions(-)