[PATCH v5 13/16] export_report: Rehabilitate script

Matthew Maurer posted 16 patches 2 months ago
There is a newer version of this series
[PATCH v5 13/16] export_report: Rehabilitate script
Posted by Matthew Maurer 2 months ago
* modules.order has .o files when in a build dir, support this
* .mod.c source layout has changed, update regexes to match
* Add a stage 3, to be more robust against additional .mod.c content

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 scripts/export_report.pl | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index feb3d5542a62..dcef915405f3 100755
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -55,6 +55,7 @@ sub collectcfiles {
     open my $fh, '< modules.order' or die "cannot open modules.order: $!\n";
     while (<$fh>) {
 	s/\.ko$/.mod.c/;
+	s/\.o$/.mod.c/;
 	push (@file, $_)
     }
     close($fh);
@@ -120,10 +121,14 @@ foreach my $thismod (@allcfiles) {
 			next;
 		}
 		if ($state == 1) {
-			$state = 2 if ($_ =~ /__attribute__\(\(section\("__versions"\)\)\)/);
+			$state = 2 if ($_ =~ /__used __section\("__versions"\)/);
 			next;
 		}
 		if ($state == 2) {
+			if ( $_ =~ /};/ ) {
+				$state = 3;
+				next;
+			}
 			if ( $_ !~ /0x[0-9a-f]+,/ ) {
 				next;
 			}
@@ -133,7 +138,7 @@ foreach my $thismod (@allcfiles) {
 			push(@{$MODULE{$thismod}} , $sym);
 		}
 	}
-	if ($state != 2) {
+	if ($state != 3) {
 		warn "WARNING:$thismod is not built with CONFIG_MODVERSIONS enabled\n";
 		$modversion_warnings++;
 	}
-- 
2.46.1.824.gd892dcdcdd-goog
Re: [PATCH v5 13/16] export_report: Rehabilitate script
Posted by Luis Chamberlain 1 month, 2 weeks ago
On Wed, Sep 25, 2024 at 11:38:28PM +0000, Matthew Maurer wrote:
> * modules.order has .o files when in a build dir, support this

The commit log is not clear, is it that it's always had *.o files, and
you're adding them now, why? Why is the .ko search now removed?

> * .mod.c source layout has changed,

When, why did this change not happen at that time?

> update regexes to match

Why did this not break anything before ? Is this fixing something, or
is it prep work?

> * Add a stage 3, to be more robust against additional .mod.c content

Future .mod.c changes?

  Luis
Re: [PATCH v5 13/16] export_report: Rehabilitate script
Posted by Matthew Maurer 1 month, 2 weeks ago
On Fri, Oct 11, 2024 at 3:13 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Sep 25, 2024 at 11:38:28PM +0000, Matthew Maurer wrote:
> > * modules.order has .o files when in a build dir, support this
>
> The commit log is not clear, is it that it's always had *.o files, and
> you're adding them now, why? Why is the .ko search now removed?

The script was broken when I found it, but it was a script that
analyzed MODVERSIONS, so I tried to ensure it would still work with my
changes. This necessitated rehabilitating it first. I did not touch
`.modules.order` files, but they contained `.o` and so this script
wouldn't run correctly.

>
> > * .mod.c source layout has changed,
>
> When, why did this change not happen at that time?

It was changed for readability [1] back in 2019. I assume the change
did not happen at that time because this script is rarely run. If we'd
prefer to discard this patch and ignore the script instead (or remove
it?), that's fine.

[1]: https://lore.kernel.org/all/20190909113423.2289-2-yamada.masahiro@socionext.com/

>
> > update regexes to match
>
> Why did this not break anything before ? Is this fixing something, or
> is it prep work?
>
> > * Add a stage 3, to be more robust against additional .mod.c content
>
> Future .mod.c changes?

The rest of this series adds additional `.mod.c` content to support
the string names. This stage 3 is intended to prevent that from
causing the script to choke.

>
>   Luis
Re: [PATCH v5 13/16] export_report: Rehabilitate script
Posted by Luis Chamberlain 1 month, 2 weeks ago
On Fri, Oct 11, 2024 at 03:22:54PM -0700, Matthew Maurer wrote:
> On Fri, Oct 11, 2024 at 3:13 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, Sep 25, 2024 at 11:38:28PM +0000, Matthew Maurer wrote:
> > > * modules.order has .o files when in a build dir, support this
> >
> > The commit log is not clear, is it that it's always had *.o files, and
> > you're adding them now, why? Why is the .ko search now removed?
> 
> The script was broken when I found it,

My point is that commit messages should describe all this.

> but it was a script that
> analyzed MODVERSIONS, so I tried to ensure it would still work with my
> changes.

OK yes please add this informaiton into the comit log.

Also expand on it as to to *why* this was not a critical issue.

> This necessitated rehabilitating it first. I did not touch
> `.modules.order` files, but they contained `.o` and so this script
> wouldn't run correctly.

Just elaborate on the commit log. We cannot guess what is happening, and
so bringing clarity into exactly what you are doing and *why* is helpful
to determine the impact of not applying this.

> > > * .mod.c source layout has changed,
> >
> > When, why did this change not happen at that time?
> 
> It was changed for readability [1] back in 2019. I assume the change
> did not happen at that time because this script is rarely run.

This is crucial information to include in the commit log. When *would*
you use it? So that use case was broken since 2019? What negative
effects has this implicated?

> If we'd
> prefer to discard this patch and ignore the script instead (or remove
> it?), that's fine.

The feedback is for you to improve the commit logs, not to tell you to
not do something. We cannot read your mind, and this smelled like a fix,
but without a true evaluation and documentation of the impact of not
having this in place. These things need to be thought out and written
in the comit log.

> 
> [1]: https://lore.kernel.org/all/20190909113423.2289-2-yamada.masahiro@socionext.com/
> 
> >
> > > update regexes to match
> >
> > Why did this not break anything before ? Is this fixing something, or
> > is it prep work?
> >
> > > * Add a stage 3, to be more robust against additional .mod.c content
> >
> > Future .mod.c changes?
> 
> The rest of this series adds additional `.mod.c` content to support
> the string names. This stage 3 is intended to prevent that from
> causing the script to choke.

The commit log should clarify all this.

  Luis