Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IRPMon log conversion fixes / overhaul #66

Merged
merged 11 commits into from
Dec 10, 2023

Conversation

iwanders
Copy link
Contributor

@iwanders iwanders commented Dec 9, 2023

As mentioned in linux-surface/kernel#144 I was running into some parsing problems with my newly obtained logs that contained the boot procedure.

This PR fixes the parsing problems, as well as introducing some other improvements:

  • I tried using the new .json output from the latest IRPMon, unfortunately, it's not conformant json, so it needs a bit of munging.
  • I added support for reading .gz files, since these logs compress to roughly 5% of the original.
  • Made a clear 'parsing' for both log and json, that outputs Irp namedtuples.
  • I overhauled the whole 'cmdbytes' approach and parsing architecture, instead of combining all data into a single byte array, we operate on the individual Irp records and advance through those, this allows printing helpful error messages that can be related to the original logged requests
  • Fixed the main issue that broke the parsing; Writes can be interleaved with Read operations. See extensive comment in the script that shows an example of this, this is mitigated by parsing the Read and Write operations independently and then recombining them into a single output list, ordered by their occurance in the original log.
  • Output is almost completely identical to the original script, the only thing that changes its the timestamps, I'm not 100% sure why this is the case, but the new approach uses the timestamp of the logged IRP that was the start of the command, so I'm fine with timestamps changing slightly from the historic output.
  • Filter only on the relevant driver and discard some IRP records that occur during boot.
  • Provide information about the log record whenever a SYN / TER isn't found.
  • All boot logs I made a few days ago parse without any errors.
  • I can understand if this overhaul is deemed to drastic, no problem.

Copy link
Member

@qzed qzed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • Output is almost completely identical to the original script, the only thing that changes its the timestamps, I'm not 100% sure why this is the case, but the new approach uses the timestamp of the logged IRP that was the start of the command, so I'm fine with timestamps changing slightly from the historic output.

I would argue that the timestamps aren't really that important. It's more about the order and being able to roughly line up actions and logged messages. So it's fine if they change a bit.

  • I can understand if this overhaul is deemed to drastic, no problem.

I don't have any issues with that. The script was a fairly quickly hacked-up way to make the logs more readable. It's nice to see it getting a bunch of improvements and fixes.

@qzed qzed merged commit de6d403 into linux-surface:master Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants