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 <gmon...@redhat.com>
>
> Some nitpicks below. But regardless:
> Reviewed-by: Nam Cao <nam...@linutronix.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
