Re: [PATCH v2 01/12] simpletrace: Improve parsing of sys.argv; fix files never closed.

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

On Tue, May 02, 2023 at 11:23:28AM +0200, Mads Ynddal wrote:
> From: Mads Ynddal <m.ynddal@samsung.com>
> 
> The arguments extracted from `sys.argv` named and unpacked to make it
> clear what the arguments are and what they're used for.
> 
> The two input files were opened, but never explicitly closed. File usage
> changed to use `with` statement to take care of this. At the same time,
> ownership of the file-object is moved up to `run` function. Secondary `open`
> inside `process` removed so there's only one place to handle `open`.
> 
> Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
> ---
>  scripts/simpletrace.py | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
> index 1f6d1ae1f3..9211caaec1 100755
> --- a/scripts/simpletrace.py
> +++ b/scripts/simpletrace.py
> @@ -9,6 +9,7 @@
>  #
>  # For help see docs/devel/tracing.rst
>  
> +import sys
>  import struct
>  import inspect
>  from tracetool import read_events, Event
> @@ -44,7 +45,6 @@ def get_record(edict, idtoname, rechdr, fobj):
>          try:
>              event = edict[name]
>          except KeyError as e:
> -            import sys
>              sys.stderr.write('%s event is logged but is not declared ' \
>                               'in the trace events file, try using ' \
>                               'trace-events-all instead.\n' % str(e))
> @@ -166,11 +166,6 @@ def end(self):
>  
>  def process(events, log, analyzer, read_header=True):
>      """Invoke an analyzer on each event in a log."""
> -    if isinstance(events, str):
> -        events = read_events(open(events, 'r'), events)
> -    if isinstance(log, str):
> -        log = open(log, 'rb')
> -
>      if read_header:
>          read_trace_header(log)

simpletrace.py is both a command-line tool and a Python module. The
Python module has a public API that people's scripts may rely on. Let's
avoid breaking API changes unless necessary so that existing scripts
that import simpletrace continue to work.

It's not very clear what is a public API in simpletrace.py, the file
should really have __all__ = ['Analyzer', 'process', 'run'].
Nevertheless, Analyzer's doc comments mention process() and the
process() function itself also has doc comments, so it's a public API.

Please drop this change to avoid breaking the public API.

>  
> @@ -223,19 +218,18 @@ def run(analyzer):
>  
>      This function is useful as a driver for simple analysis scripts.  More
>      advanced scripts will want to call process() instead."""
> -    import sys
> -
> -    read_header = True
> -    if len(sys.argv) == 4 and sys.argv[1] == '--no-header':
> -        read_header = False
> -        del sys.argv[1]
> -    elif len(sys.argv) != 3:
> -        sys.stderr.write('usage: %s [--no-header] <trace-events> ' \
> -                         '<trace-file>\n' % sys.argv[0])
> +
> +    try:
> +        # NOTE: See built-in `argparse` module for a more robust cli interface
> +        *no_header, trace_event_path, trace_file_path = sys.argv[1:]
> +        assert no_header == [] or no_header == ['--no-header'], 'Invalid no-header argument'
> +    except (AssertionError, ValueError):
> +        sys.stderr.write(f'usage: {sys.argv[0]} [--no-header] <trace-events> <trace-file>\n')
>          sys.exit(1)
>  
> -    events = read_events(open(sys.argv[1], 'r'), sys.argv[1])
> -    process(events, sys.argv[2], analyzer, read_header=read_header)
> +    with open(trace_event_path, 'r') as events_fobj, open(trace_file_path, 'rb') as log_fobj:
> +        events = read_events(events_fobj, trace_event_path)
> +        process(events, log_fobj, analyzer, read_header=not no_header)
>  
>  if __name__ == '__main__':
>      class Formatter(Analyzer):
> -- 
> 2.38.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]