linux-kernel - Re: [PATCH v4 08/14] verification/rvgen: Organise Kconfig entries for nested monitors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <85876b4f36c4230c0be34fb0b95f539179dcd445.camel@redhat.com>
Date: Mon, 21 Jul 2025 17:17:17 +0200
From: Gabriele Monaco <gmonaco@...hat.com>
To: Nam Cao <namcao@...utronix.de>
Cc: linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>, 
 Masami Hiramatsu <mhiramat@...nel.org>, linux-trace-kernel@...r.kernel.org,
 Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Tomas Glozar <tglozar@...hat.com>, Juri Lelli <jlelli@...hat.com>,  Clark
 Williams <williams@...hat.com>, John Kacur <jkacur@...hat.com>
Subject: Re: [PATCH v4 08/14] verification/rvgen: Organise Kconfig entries
 for nested monitors

On Mon, 2025-07-21 at 16:38 +0200, Nam Cao wrote:
> On Mon, Jul 21, 2025 at 10:23:18AM +0200, Gabriele Monaco wrote:
> > The current behaviour of rvgen when running with the -a option is
> > to append the necessary lines at the end of the configuration for
> > Kconfig, Makefile and tracepoints.
> > 
> > Adapt rvgen to look for a different marker for nested monitors in
> > the
> > Kconfig file and append the line right after the last sibling,
> > instead
> > of the last monitor.
> > Also add the marker when creating a new parent monitor.
> > 
> > Signed-off-by: Gabriele Monaco <gmonaco@...hat.com>
> 
> Some nitpicks below. But regardless:
> Reviewed-by: Nam Cao <namcao@...utronix.de>
> 
> > -    def __patch_file(self, file, marker, line):
> > +    def _patch_file(self, file, marker, line):
> > +        assert(self.auto_patch)
> 
> Nit: follows PEP8 unless there is a reason not to: assert
> self.auto_patch

Well, all the python best practices I follow are the ones reported by
pylint, I guess I need to configure it to follow those.
Thanks for pointing that out!

> 
> >          file_to_patch = os.path.join(self.rv_dir, file)
> >          content = self._read_file(file_to_patch)
> >          content = content.replace(marker, line + "\n" + marker)
> > @@ -146,7 +147,7 @@ class RVGenerator:
> >      def fill_tracepoint_tooltip(self):
> >          monitor_class_type = self.fill_monitor_class_type()
> >          if self.auto_patch:
> > -            self.__patch_file("rv_trace.h",
> > +            self._patch_file("rv_trace.h",
> >                              "// Add new monitors based on
> > CONFIG_%s here" % monitor_class_type,
> >                              "#include <monitors/%s/%s_trace.h>" %
> > (self.name, self.name))
> >              return "  - Patching %s/rv_trace.h, double check the
> > result" % self.rv_dir
> > @@ -156,10 +157,15 @@ Add this line where other tracepoints are
> > included and %s is defined:
> >  #include <monitors/%s/%s_trace.h>
> >  """ % (self.rv_dir, monitor_class_type, self.name, self.name)
> >  
> > +    def _container_marker(self, container = None) -> str:
> > +        return "# Add new %smonitors here" % (container + " "
> > +                                              if container else
> > "")
> 
> PEP8: container=None
> 
> And this function name is misleading, it is not marker for only
> containers.
> Perhaps _kconfig_marker() is more appropriate.

Good point.

Thanks,
Gabriele