[PATCH] perf test pmu: Set uninitialized PMU alias to null

public inbox for linux-perf-users@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH] perf test pmu: Set uninitialized PMU alias to null
@ 2024-08-08 10:37 vmolnaro
  2024-08-08 14:01 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: vmolnaro @ 2024-08-08 10:37 UTC (permalink / raw)
  To: linux-perf-users, acme, acme; +Cc: mpetlan, james.clark, rstoyano

From: Veronika Molnarova <vmolnaro@redhat.com>

Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
a test case "PMU cmdline match" that covers PMU name wildcard support
provided by function perf_pmu__match(). The test works with a wide
range of supported combinations of PMU name matching but omits the case
that if the perf_pmu__match() cannot match the PMU name to the wildcard,
it tries to match its alias. However, this variable is not set up,
causing the test case to fail when run with subprocesses or to segfault
if run as a single process.

./perf test -vv 9
  9: Sysfs PMU tests                                                 :
  9.1: Parsing with PMU format directory                             : Ok
  9.2: Parsing with PMU event                                        : Ok
  9.3: PMU event names                                               : Ok
  9.4: PMU name combining                                            : Ok
  9.5: PMU name comparison                                           : Ok
  9.6: PMU cmdline match                                             : FAILED!

./perf test -F 9
  9.1: Parsing with PMU format directory                             : Ok
  9.2: Parsing with PMU event                                        : Ok
  9.3: PMU event names                                               : Ok
  9.4: PMU name combining                                            : Ok
  9.5: PMU name comparison                                           : Ok
Segmentation fault (core dumped)

Initialize the PMU alias to null for all tests of perf_pmu__match()
as this functionality is not being tested and the alias matching works
exactly the same as the matching of the PMU name.

./perf test -F 9
  9.1: Parsing with PMU format directory                             : Ok
  9.2: Parsing with PMU event                                        : Ok
  9.3: PMU event names                                               : Ok
  9.4: PMU name combining                                            : Ok
  9.5: PMU name comparison                                           : Ok
  9.6: PMU cmdline match                                             : Ok

Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
---
 tools/perf/tests/pmu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index 0b2f04a55d7b..a4730b5dc0d9 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
 /**
  * Test perf_pmu__match() that's used to search for a PMU given a name passed
  * on the command line. The name that's passed may also be a filename type glob
- * match.
+ * match. If the name does not match, perf_pmu__match() attempts to match the
+ * alias of the PMU, if provided.
  */
 static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
 	struct perf_pmu test_pmu;
+	test_pmu.alias_name = NULL;
 
 	test_pmu.name = "pmuname";
 	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf test pmu: Set uninitialized PMU alias to null
  2024-08-08 10:37 [PATCH] perf test pmu: Set uninitialized PMU alias to null vmolnaro
@ 2024-08-08 14:01 ` Arnaldo Carvalho de Melo
  2024-08-08 14:12   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-08-08 14:01 UTC (permalink / raw)
  To: vmolnaro; +Cc: linux-perf-users, acme, mpetlan, james.clark, rstoyano

On Thu, Aug 08, 2024 at 12:37:49PM +0200, vmolnaro@redhat.com wrote:
> From: Veronika Molnarova <vmolnaro@redhat.com>
> 
> Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
> a test case "PMU cmdline match" that covers PMU name wildcard support
> provided by function perf_pmu__match(). The test works with a wide
> range of supported combinations of PMU name matching but omits the case
> that if the perf_pmu__match() cannot match the PMU name to the wildcard,
> it tries to match its alias. However, this variable is not set up,
> causing the test case to fail when run with subprocesses or to segfault
> if run as a single process.
> 
> ./perf test -vv 9
>   9: Sysfs PMU tests                                                 :
>   9.1: Parsing with PMU format directory                             : Ok
>   9.2: Parsing with PMU event                                        : Ok
>   9.3: PMU event names                                               : Ok
>   9.4: PMU name combining                                            : Ok
>   9.5: PMU name comparison                                           : Ok
>   9.6: PMU cmdline match                                             : FAILED!
> 
> ./perf test -F 9
>   9.1: Parsing with PMU format directory                             : Ok
>   9.2: Parsing with PMU event                                        : Ok
>   9.3: PMU event names                                               : Ok
>   9.4: PMU name combining                                            : Ok
>   9.5: PMU name comparison                                           : Ok
> Segmentation fault (core dumped)
> 
> Initialize the PMU alias to null for all tests of perf_pmu__match()
> as this functionality is not being tested and the alias matching works
> exactly the same as the matching of the PMU name.
> 
> ./perf test -F 9
>   9.1: Parsing with PMU format directory                             : Ok
>   9.2: Parsing with PMU event                                        : Ok
>   9.3: PMU event names                                               : Ok
>   9.4: PMU name combining                                            : Ok
>   9.5: PMU name comparison                                           : Ok
>   9.6: PMU cmdline match                                             : Ok
> 
> Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> ---
>  tools/perf/tests/pmu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 0b2f04a55d7b..a4730b5dc0d9 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
>  /**
>   * Test perf_pmu__match() that's used to search for a PMU given a name passed
>   * on the command line. The name that's passed may also be a filename type glob
> - * match.
> + * match. If the name does not match, perf_pmu__match() attempts to match the
> + * alias of the PMU, if provided.
>   */
>  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>  {
>  	struct perf_pmu test_pmu;
> +	test_pmu.alias_name = NULL;

We can do a bit more future proofing by instead doing:

	struct perf_pmu test_pmu = {
		.name = "pmuname",
	};

So that all the other fields are initialized to zero, ok? I'll do this
change and make a note in the commit, lemme know if you disagree.

- Arnaldo
  
>  	test_pmu.name = "pmuname";
>  	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf test pmu: Set uninitialized PMU alias to null
  2024-08-08 14:01 ` Arnaldo Carvalho de Melo
@ 2024-08-08 14:12   ` Arnaldo Carvalho de Melo
  2024-08-08 14:42     ` Veronika Molnarova
  0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-08-08 14:12 UTC (permalink / raw)
  To: vmolnaro; +Cc: linux-perf-users, acme, mpetlan, james.clark, rstoyano

On Thu, Aug 08, 2024 at 11:01:28AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Aug 08, 2024 at 12:37:49PM +0200, vmolnaro@redhat.com wrote:
> > From: Veronika Molnarova <vmolnaro@redhat.com>
> > 
> > Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
> > a test case "PMU cmdline match" that covers PMU name wildcard support
> > provided by function perf_pmu__match(). The test works with a wide
> > range of supported combinations of PMU name matching but omits the case
> > that if the perf_pmu__match() cannot match the PMU name to the wildcard,
> > it tries to match its alias. However, this variable is not set up,
> > causing the test case to fail when run with subprocesses or to segfault
> > if run as a single process.
> > 
> > ./perf test -vv 9
> >   9: Sysfs PMU tests                                                 :
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> >   9.6: PMU cmdline match                                             : FAILED!
> > 
> > ./perf test -F 9
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> > Segmentation fault (core dumped)
> > 
> > Initialize the PMU alias to null for all tests of perf_pmu__match()
> > as this functionality is not being tested and the alias matching works
> > exactly the same as the matching of the PMU name.
> > 
> > ./perf test -F 9
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> >   9.6: PMU cmdline match                                             : Ok
> > 
> > Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
> > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> > ---
> >  tools/perf/tests/pmu.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> > index 0b2f04a55d7b..a4730b5dc0d9 100644
> > --- a/tools/perf/tests/pmu.c
> > +++ b/tools/perf/tests/pmu.c
> > @@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> >  /**
> >   * Test perf_pmu__match() that's used to search for a PMU given a name passed
> >   * on the command line. The name that's passed may also be a filename type glob
> > - * match.
> > + * match. If the name does not match, perf_pmu__match() attempts to match the
> > + * alias of the PMU, if provided.
> >   */
> >  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> >  {
> >  	struct perf_pmu test_pmu;
> > +	test_pmu.alias_name = NULL;
> 
> We can do a bit more future proofing by instead doing:
> 
> 	struct perf_pmu test_pmu = {
> 		.name = "pmuname",
> 	};
> 
> So that all the other fields are initialized to zero, ok? I'll do this
> change and make a note in the commit, lemme know if you disagree.

Nah, tried with:

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index a4730b5dc0d9259d..c9d4ed6d684551c1 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -457,11 +457,11 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
  * alias of the PMU, if provided.
  */
 static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
-{
-	struct perf_pmu test_pmu;
-	test_pmu.alias_name = NULL;
 
-	test_pmu.name = "pmuname";
+	struct perf_pmu test_pmu = {
+		.name = "pmuname",
+	};
+
 	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
 	TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
 	TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),	     false);

But those macros are not liking it:

tests/pmu.c: In function ‘test__pmu_match’:
tests/pmu.c:461:16: error: parameter ‘test_pmu’ is initialized
  461 |         struct perf_pmu test_pmu = {
      |                ^~~~~~~~
In file included from tests/pmu.c:7:
tests/tests.h:22:1: error: expected declaration specifiers before ‘do’
   22 | do {                                                                     \
      | ^~
tests/pmu.c:465:9: note: in expansion of macro ‘TEST_ASSERT_EQUAL’
  465 |         TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
      |         ^~~~~~~~~~~~~~~~~
tests/tests.h:28:3: error: expected declaration specifiers before ‘while’
   28 | } while (0)
      |   ^~~~~

So I'll not spend more time here, I'm keeping your patch as-is.

Thanks and sorry for the noise :-)

- Arnaldo

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf test pmu: Set uninitialized PMU alias to null
  2024-08-08 14:12   ` Arnaldo Carvalho de Melo
@ 2024-08-08 14:42     ` Veronika Molnarova
  2024-08-12 12:56       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Veronika Molnarova @ 2024-08-08 14:42 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-perf-users, acme, mpetlan, james.clark, rstoyano



On 8/8/24 16:12, Arnaldo Carvalho de Melo wrote:
> On Thu, Aug 08, 2024 at 11:01:28AM -0300, Arnaldo Carvalho de Melo wrote:
>> On Thu, Aug 08, 2024 at 12:37:49PM +0200, vmolnaro@redhat.com wrote:
>>> From: Veronika Molnarova <vmolnaro@redhat.com>
>>>
>>> Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
>>> a test case "PMU cmdline match" that covers PMU name wildcard support
>>> provided by function perf_pmu__match(). The test works with a wide
>>> range of supported combinations of PMU name matching but omits the case
>>> that if the perf_pmu__match() cannot match the PMU name to the wildcard,
>>> it tries to match its alias. However, this variable is not set up,
>>> causing the test case to fail when run with subprocesses or to segfault
>>> if run as a single process.
>>>
>>> ./perf test -vv 9
>>>   9: Sysfs PMU tests                                                 :
>>>   9.1: Parsing with PMU format directory                             : Ok
>>>   9.2: Parsing with PMU event                                        : Ok
>>>   9.3: PMU event names                                               : Ok
>>>   9.4: PMU name combining                                            : Ok
>>>   9.5: PMU name comparison                                           : Ok
>>>   9.6: PMU cmdline match                                             : FAILED!
>>>
>>> ./perf test -F 9
>>>   9.1: Parsing with PMU format directory                             : Ok
>>>   9.2: Parsing with PMU event                                        : Ok
>>>   9.3: PMU event names                                               : Ok
>>>   9.4: PMU name combining                                            : Ok
>>>   9.5: PMU name comparison                                           : Ok
>>> Segmentation fault (core dumped)
>>>
>>> Initialize the PMU alias to null for all tests of perf_pmu__match()
>>> as this functionality is not being tested and the alias matching works
>>> exactly the same as the matching of the PMU name.
>>>
>>> ./perf test -F 9
>>>   9.1: Parsing with PMU format directory                             : Ok
>>>   9.2: Parsing with PMU event                                        : Ok
>>>   9.3: PMU event names                                               : Ok
>>>   9.4: PMU name combining                                            : Ok
>>>   9.5: PMU name comparison                                           : Ok
>>>   9.6: PMU cmdline match                                             : Ok
>>>
>>> Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
>>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
>>> ---
>>>  tools/perf/tests/pmu.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
>>> index 0b2f04a55d7b..a4730b5dc0d9 100644
>>> --- a/tools/perf/tests/pmu.c
>>> +++ b/tools/perf/tests/pmu.c
>>> @@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
>>>  /**
>>>   * Test perf_pmu__match() that's used to search for a PMU given a name passed
>>>   * on the command line. The name that's passed may also be a filename type glob
>>> - * match.
>>> + * match. If the name does not match, perf_pmu__match() attempts to match the
>>> + * alias of the PMU, if provided.
>>>   */
>>>  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>>>  {
>>>  	struct perf_pmu test_pmu;
>>> +	test_pmu.alias_name = NULL;
>>
>> We can do a bit more future proofing by instead doing:
>>
>> 	struct perf_pmu test_pmu = {
>> 		.name = "pmuname",
>> 	};
>>
>> So that all the other fields are initialized to zero, ok? I'll do this
>> change and make a note in the commit, lemme know if you disagree.
> 
> Nah, tried with:
> 
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index a4730b5dc0d9259d..c9d4ed6d684551c1 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -457,11 +457,11 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
>   * alias of the PMU, if provided.
>   */
>  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> -{
> -	struct perf_pmu test_pmu;
> -	test_pmu.alias_name = NULL;
>  
> -	test_pmu.name = "pmuname";
> +	struct perf_pmu test_pmu = {
> +		.name = "pmuname",
> +	};
> +
>  	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
>  	TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
>  	TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),	     false);
> 
Isn't the error caused by a missing curly bracket at the start of the function?
Tried it and works just fine for me.

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index a4730b5dc0d9..be18506f6a24 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
  */
 static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
-       struct perf_pmu test_pmu;
-       test_pmu.alias_name = NULL;
+       struct perf_pmu test_pmu = {
+               .name = "pmuname",
+       };
 
-       test_pmu.name = "pmuname";
        TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
        TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
        TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),        false);

./perf test -F 9
  9.1: Parsing with PMU format directory                             : Ok
  9.2: Parsing with PMU event                                        : Ok
  9.3: PMU event names                                               : Ok
  9.4: PMU name combining                                            : Ok
  9.5: PMU name comparison                                           : Ok
  9.6: PMU cmdline match                                             : Ok

> But those macros are not liking it:
> 
> tests/pmu.c: In function ‘test__pmu_match’:
> tests/pmu.c:461:16: error: parameter ‘test_pmu’ is initialized
>   461 |         struct perf_pmu test_pmu = {
>       |                ^~~~~~~~
> In file included from tests/pmu.c:7:
> tests/tests.h:22:1: error: expected declaration specifiers before ‘do’
>    22 | do {                                                                     \
>       | ^~
> tests/pmu.c:465:9: note: in expansion of macro ‘TEST_ASSERT_EQUAL’
>   465 |         TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
>       |         ^~~~~~~~~~~~~~~~~
> tests/tests.h:28:3: error: expected declaration specifiers before ‘while’
>    28 | } while (0)
>       |   ^~~~~
> 
> So I'll not spend more time here, I'm keeping your patch as-is.
> 
> Thanks and sorry for the noise :-)
> 
> - Arnaldo
> 


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf test pmu: Set uninitialized PMU alias to null
  2024-08-08 14:42     ` Veronika Molnarova
@ 2024-08-12 12:56       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-08-12 12:56 UTC (permalink / raw)
  To: Veronika Molnarova; +Cc: linux-perf-users, acme, mpetlan, james.clark, rstoyano

On Thu, Aug 08, 2024 at 04:42:30PM +0200, Veronika Molnarova wrote:
> On 8/8/24 16:12, Arnaldo Carvalho de Melo wrote:
> > On Thu, Aug 08, 2024 at 11:01:28AM -0300, Arnaldo Carvalho de Melo wrote:
> >> On Thu, Aug 08, 2024 at 12:37:49PM +0200, vmolnaro@redhat.com wrote:
> >>> From: Veronika Molnarova <vmolnaro@redhat.com>
> >>>
> >>> Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
> >>> a test case "PMU cmdline match" that covers PMU name wildcard support
> >>> provided by function perf_pmu__match(). The test works with a wide
> >>> range of supported combinations of PMU name matching but omits the case
> >>> that if the perf_pmu__match() cannot match the PMU name to the wildcard,
> >>> it tries to match its alias. However, this variable is not set up,
> >>> causing the test case to fail when run with subprocesses or to segfault
> >>> if run as a single process.
> >>>
> >>> ./perf test -vv 9
> >>>   9: Sysfs PMU tests                                                 :
> >>>   9.1: Parsing with PMU format directory                             : Ok
> >>>   9.2: Parsing with PMU event                                        : Ok
> >>>   9.3: PMU event names                                               : Ok
> >>>   9.4: PMU name combining                                            : Ok
> >>>   9.5: PMU name comparison                                           : Ok
> >>>   9.6: PMU cmdline match                                             : FAILED!
> >>>
> >>> ./perf test -F 9
> >>>   9.1: Parsing with PMU format directory                             : Ok
> >>>   9.2: Parsing with PMU event                                        : Ok
> >>>   9.3: PMU event names                                               : Ok
> >>>   9.4: PMU name combining                                            : Ok
> >>>   9.5: PMU name comparison                                           : Ok
> >>> Segmentation fault (core dumped)
> >>>
> >>> Initialize the PMU alias to null for all tests of perf_pmu__match()
> >>> as this functionality is not being tested and the alias matching works
> >>> exactly the same as the matching of the PMU name.
> >>>
> >>> ./perf test -F 9
> >>>   9.1: Parsing with PMU format directory                             : Ok
> >>>   9.2: Parsing with PMU event                                        : Ok
> >>>   9.3: PMU event names                                               : Ok
> >>>   9.4: PMU name combining                                            : Ok
> >>>   9.5: PMU name comparison                                           : Ok
> >>>   9.6: PMU cmdline match                                             : Ok
> >>>
> >>> Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
> >>> Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> >>> ---
> >>>  tools/perf/tests/pmu.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> >>> index 0b2f04a55d7b..a4730b5dc0d9 100644
> >>> --- a/tools/perf/tests/pmu.c
> >>> +++ b/tools/perf/tests/pmu.c
> >>> @@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> >>>  /**
> >>>   * Test perf_pmu__match() that's used to search for a PMU given a name passed
> >>>   * on the command line. The name that's passed may also be a filename type glob
> >>> - * match.
> >>> + * match. If the name does not match, perf_pmu__match() attempts to match the
> >>> + * alias of the PMU, if provided.
> >>>   */
> >>>  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> >>>  {
> >>>  	struct perf_pmu test_pmu;
> >>> +	test_pmu.alias_name = NULL;
> >>
> >> We can do a bit more future proofing by instead doing:
> >>
> >> 	struct perf_pmu test_pmu = {
> >> 		.name = "pmuname",
> >> 	};
> >>
> >> So that all the other fields are initialized to zero, ok? I'll do this
> >> change and make a note in the commit, lemme know if you disagree.
> > 
> > Nah, tried with:
> > 
> > diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> > index a4730b5dc0d9259d..c9d4ed6d684551c1 100644
> > --- a/tools/perf/tests/pmu.c
> > +++ b/tools/perf/tests/pmu.c
> > @@ -457,11 +457,11 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> >   * alias of the PMU, if provided.
> >   */
> >  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> > -{
> > -	struct perf_pmu test_pmu;
> > -	test_pmu.alias_name = NULL;
> >  
> > -	test_pmu.name = "pmuname";
> > +	struct perf_pmu test_pmu = {
> > +		.name = "pmuname",
> > +	};
> > +
> >  	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
> >  	TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
> >  	TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),	     false);
> > 
> Isn't the error caused by a missing curly bracket at the start of the function?
> Tried it and works just fine for me.

Right, well spotted! My bad :-\

Anyway, it is already in perf-tools-next, so I can't change it anymore,
so I'll add a patch on top of it doing this more future-proof approach.

Thanks for finding my mistake!

- Arnaldo
 
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index a4730b5dc0d9..be18506f6a24 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
>   */
>  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>  {
> -       struct perf_pmu test_pmu;
> -       test_pmu.alias_name = NULL;
> +       struct perf_pmu test_pmu = {
> +               .name = "pmuname",
> +       };
>  
> -       test_pmu.name = "pmuname";
>         TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
>         TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
>         TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),        false);
> 
> ./perf test -F 9
>   9.1: Parsing with PMU format directory                             : Ok
>   9.2: Parsing with PMU event                                        : Ok
>   9.3: PMU event names                                               : Ok
>   9.4: PMU name combining                                            : Ok
>   9.5: PMU name comparison                                           : Ok
>   9.6: PMU cmdline match                                             : Ok
> 
> > But those macros are not liking it:
> > 
> > tests/pmu.c: In function ‘test__pmu_match’:
> > tests/pmu.c:461:16: error: parameter ‘test_pmu’ is initialized
> >   461 |         struct perf_pmu test_pmu = {
> >       |                ^~~~~~~~
> > In file included from tests/pmu.c:7:
> > tests/tests.h:22:1: error: expected declaration specifiers before ‘do’
> >    22 | do {                                                                     \
> >       | ^~
> > tests/pmu.c:465:9: note: in expansion of macro ‘TEST_ASSERT_EQUAL’
> >   465 |         TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
> >       |         ^~~~~~~~~~~~~~~~~
> > tests/tests.h:28:3: error: expected declaration specifiers before ‘while’
> >    28 | } while (0)
> >       |   ^~~~~
> > 
> > So I'll not spend more time here, I'm keeping your patch as-is.
> > 
> > Thanks and sorry for the noise :-)
> > 
> > - Arnaldo
> > 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-08-12 12:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-08 10:37 [PATCH] perf test pmu: Set uninitialized PMU alias to null vmolnaro
2024-08-08 14:01 ` Arnaldo Carvalho de Melo
2024-08-08 14:12   ` Arnaldo Carvalho de Melo
2024-08-08 14:42     ` Veronika Molnarova
2024-08-12 12:56       ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox