Re: [PATCH v2 2/4] scripts/replay-dump.py: Update to current rr record format

["Nicholas Piggin" <npiggin@gmail.com>]

On Wed Jan 31, 2024 at 7:36 AM AEST, John Snow wrote:
> On Thu, Jan 25, 2024 at 11:09 AM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > The v12 format support for replay-dump has a few issues still. This
> > fixes async decoding; adds event, shutdown, and end decoding; fixes
> > audio in / out events, fixes checkpoint checking of following async
> > events.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  scripts/replay-dump.py | 132 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 98 insertions(+), 34 deletions(-)
> >
> > diff --git a/scripts/replay-dump.py b/scripts/replay-dump.py
> > index d668193e79..35732da08f 100755
> > --- a/scripts/replay-dump.py
> > +++ b/scripts/replay-dump.py
> > @@ -20,6 +20,7 @@
> >
> >  import argparse
> >  import struct
> > +import os
> >  from collections import namedtuple
> >  from os import path
> >
> > @@ -63,6 +64,10 @@ def read_byte(fin):
> >      "Read a single byte"
> >      return struct.unpack('>B', fin.read(1))[0]
> >
> > +def read_bytes(fin, nr):
> > +    "Read a nr bytes"
>
> Existing problem in this file, but please use """triple quotes""" for
> docstrings.

Just coming back to this, sorry, was struggling a bit with ppc merge :/

> > +    return fin.read(nr)
> > +
>
> Does it really save a lot of typing to alias fin.read(1) to
> read_bytes(fin, 1) ...?

Not really, I'll squash it.

>
> >  def read_event(fin):
> >      "Read a single byte event, but save some state"
> >      if replay_state.already_read:
> > @@ -134,6 +139,18 @@ def swallow_async_qword(eid, name, dumpfile):
> >      print("  %s(%d) @ %d" % (name, eid, step_id))
> >      return True
> >
> > +def swallow_bytes(eid, name, dumpfile, nr):
> > +    "Swallow nr bytes of data without looking at it"
> > +    dumpfile.seek(nr, os.SEEK_CUR)
> > +    return True
> > +
>
> Why bother returning a bool if it's not based on any condition? Add an
> error check or just drop the return value.
>
> > +def decode_exception(eid, name, dumpfile):
> > +    print_event(eid, name)
> > +    return True
> > +
>
> I suppose in this case, the return is to fit a common signature.

Yes that's why I did it, but it actually can't fit in the normal
decoder pattern because a nr has to be supplied as well. I'll
change it as you say.

Thanks,
Nick