Message-ID: <fb6b81910e32dbf7aa16e1961dc1dca0a9bfbf57.camel@redhat.com>
Date: Tue, 20 Jan 2026 16:08:21 +0100
From: Gabriele Monaco <gmonaco@...hat.com>
To: Wander Lairson Costa <wander@...hat.com>
Cc: Nam Cao <namcao@...utronix.de>, Steven Rostedt <rostedt@...dmis.org>, 
 open list <linux-kernel@...r.kernel.org>, "open list:RUNTIME VERIFICATION
 (RV)"	 <linux-trace-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/26] rv/rvgen: introduce AutomataError exception class

On Tue, 2026-01-20 at 09:39 -0300, Wander Lairson Costa wrote:
> On Tue, Jan 20, 2026 at 08:33:10AM +0100, Gabriele Monaco wrote:
> > I agree catching all exceptions like this is quite detrimental while
> > debugging,
> > but I see the original intent.
> > When you run commands written in python, you normally don't expect them to
> > blurt
> > a stack trace when doing relatively normal things, like opening a wrong
> > file.
> > Sure that might be useful when debugging, but for a user-facing tool we want
> > to
> > write a meaningful error message and gracefully fail.
> > 
> 
> One option I thought was to keep it as it is but adding a --debug option
> which would reraise the exception and then print the stack trace.
> But as the users are developers themselves, leaving the exception
> unchaught would help them identify the error (although I am strongly
> against doing this in server side code). Another reason is the case
> when the code itself has a bug. That would facilitate bug reports.

That could be a good tradeoff. Users are developer but (although I'm not sure if
it really happened yet) are not the rvgen developers, they don't need to know
where exactly the code complained, unless it really broke.
All errors that are expected (OSError or wrong format) should have a meaningful
message for the user, I believe by doing that we'd have a pretty clear idea
where the error came from in the code too (e.g. event parsing, opening a file,
etc.).

If the code has a bug, then yes we should throw the exception as is, that's why
I think it's good not to catch Exception, but to catch only the few exceptions
we know can happen, all others would be bugs.

> 
> Perhaps we could catch more specific exceptions that would indicate a
> problem with the dot files instead of Exception. Like
> 
> try: ...
> except AutomataError as e:
>     print(f"There was a problem processing {dot_file}: {str(e)}",
>           file=sys.stderr)
>     sys.exit(1)
> 
> Which would be a common case. And leaving other types of exceptions
> unchaught.

Yeah pretty much

> 
> > Other story is when the exception is something unexpected (that's why
> > leaving a
> > generic Exception here is bad).
> > 
> > >      print("Writing the monitor into the directory %s" % monitor.name)
> > >      monitor.print_files()
> > > diff --git a/tools/verification/rvgen/rvgen/automata.py
> > > b/tools/verification/rvgen/rvgen/automata.py
> > > index d9a3fe2b74bf2..8d88c3b65d00d 100644
> > > --- a/tools/verification/rvgen/rvgen/automata.py
> > > +++ b/tools/verification/rvgen/rvgen/automata.py
> > > @@ -10,6 +10,13 @@
> > >  
> > >  import ntpath
> > >  
> > > +class AutomataError(OSError):
> > > +    """Exception raised for errors in automata parsing and validation.
> > > +
> > > +    Raised when DOT file processing fails due to invalid format, I/O
> > > errors,
> > > +    or malformed automaton definitions.
> > > +    """
> > > +
> > 
> > I'm not quite familiar with modern python best practices (so again, take my
> > comments with a grain of salt ;) ), but what is the advantage of using this
> > custom exception instead of using pre-existing specific exception types?
> > 
> > Although the difference is minimal, here you're throwing an OSError for
> > something that quite isn't (e.g. wrong format for the dot file).
> > A ValueError feels more appropriate to me in most of the instances here.
> > 
> > All in all, I would do something like:
> > * throw a ValueError (or a custom one based on that) whenever we expect
> > wrong
> > data not dependent on OS features
> > * throw OSError whenever that was the exception, perhaps changing the
> > message to
> > something more meaningful to us (like you're already doing here)
> > * intercept only those errors in main.py and print the message without stack
> > trace (if the message is clear enough we shouldn't need it).
> > 
> > Does it make sense to you?
> > 
> 
> The reasoning behind specific exception types is to allow the calling code
> process diferent exceptions in more specialized code paths, like the
> example above.
> 
> AutomataError could derive from both OSError and ValueError.
> 
> class AutomataError(OSError, ValueError): ...
> 
> Which would address your (valid) point. This way, the calling code could
> either process specific automata related errors with AutomataError, or
> handle general file error, like that:
> 
> try...
> except OSError as e:
>     print(f"File error: {str(e)", file=sys.stderr)
> except AutomataError as e:
>     print(f"Ill formed dot file: {str(e)", file=stderr)
> 

Yes! That's exactly what I mean, those are exceptions we expect, so no splat,
all others can just propagate.

Thanks,
Gabriele

> > Thanks,
> > Gabriele
> > 
> > >  class Automata:
> > >      """Automata class: Reads a dot file and part it as an automata.
> > >  
> > > @@ -32,11 +39,11 @@ class Automata:
> > >          basename = ntpath.basename(self.__dot_path)
> > >          if not basename.endswith(".dot") and not
> > > basename.endswith(".gv"):
> > >              print("not a dot file")
> > > -            raise Exception("not a dot file: %s" % self.__dot_path)
> > > +            raise AutomataError("not a dot file: %s" % self.__dot_path)
> > >  
> > >          model_name = ntpath.splitext(basename)[0]
> > >          if model_name.__len__() == 0:
> > > -            raise Exception("not a dot file: %s" % self.__dot_path)
> > > +            raise AutomataError("not a dot file: %s" % self.__dot_path)
> > >  
> > >          return model_name
> > >  
> > > @@ -45,8 +52,8 @@ class Automata:
> > >          dot_lines = []
> > >          try:
> > >              dot_file = open(self.__dot_path)
> > > -        except:
> > > -            raise Exception("Cannot open the file: %s" % self.__dot_path)
> > > +        except OSError as exc:
> > > +            raise AutomataError(f"Cannot open the file:
> > > {self.__dot_path}")
> > > from exc
> > >  
> > >          dot_lines = dot_file.read().splitlines()
> > >          dot_file.close()
> > > @@ -55,7 +62,7 @@ class Automata:
> > >          line = dot_lines[cursor].split()
> > >  
> > >          if (line[0] != "digraph") and (line[1] != "state_automaton"):
> > > -            raise Exception("Not a valid .dot format: %s" %
> > > self.__dot_path)
> > > +            raise AutomataError("Not a valid .dot format: %s" %
> > > self.__dot_path)
> > >          else:
> > >              cursor += 1
> > >          return dot_lines
> > > diff --git a/tools/verification/rvgen/rvgen/dot2c.py
> > > b/tools/verification/rvgen/rvgen/dot2c.py
> > > index b9b6f14cc536a..1a1770e7f20c0 100644
> > > --- a/tools/verification/rvgen/rvgen/dot2c.py
> > > +++ b/tools/verification/rvgen/rvgen/dot2c.py
> > > @@ -13,7 +13,7 @@
> > >  # For further information, see:
> > >  #   Documentation/trace/rv/deterministic_automata.rst
> > >  
> > > -from .automata import Automata
> > > +from .automata import Automata, AutomataError
> > >  
> > >  class Dot2c(Automata):
> > >      enum_suffix = ""
> > > @@ -93,7 +93,7 @@ class Dot2c(Automata):
> > >              min_type = "unsigned int"
> > >  
> > >          if self.states.__len__() > 1000000:
> > > -            raise Exception("Too many states: %d" %
> > > self.states.__len__())
> > > +            raise AutomataError("Too many states: %d" %
> > > self.states.__len__())
> > >  
> > >          return min_type
> > >  
> > > diff --git a/tools/verification/rvgen/rvgen/generator.py
> > > b/tools/verification/rvgen/rvgen/generator.py
> > > index 3441385c11770..a7bee6b1ea70c 100644
> > > --- a/tools/verification/rvgen/rvgen/generator.py
> > > +++ b/tools/verification/rvgen/rvgen/generator.py
> > > @@ -51,10 +51,7 @@ class RVGenerator:
> > >          raise FileNotFoundError("Could not find the rv directory, do you
> > > have
> > > the kernel source installed?")
> > >  
> > >      def _read_file(self, path):
> > > -        try:
> > > -            fd = open(path, 'r')
> > > -        except OSError:
> > > -            raise Exception("Cannot open the file: %s" % path)
> > > +        fd = open(path, 'r')
> > >  
> > >          content = fd.read()
> > >  
> > > @@ -65,7 +62,7 @@ class RVGenerator:
> > >          try:
> > >              path = os.path.join(self.abs_template_dir, file)
> > >              return self._read_file(path)
> > > -        except Exception:
> > > +        except OSError:
> > >              # Specific template file not found. Try the generic template
> > > file
> > > in the template/
> > >              # directory, which is one level up
> > >              path = os.path.join(self.abs_template_dir, "..", file)
> > 

Powered by blists - more mailing lists