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

fbptcat: dump raw contents of FBPT table #2762

Merged
merged 3 commits into from
Nov 4, 2023

Conversation

princegeutler
Copy link
Contributor

@princegeutler princegeutler commented Sep 12, 2023

fbptcat: dump raw contents of FBPT table
FBPT = Firmware Basic Performance Boot Table

  • fbptcat is a shell command for printing the raw contents of
    Firmware Basic Performance Boot Table (FBPT) within ACPI FPDT

  • New FPDT package created to contain functions concerning
    all tables and structures within ACPI FPDT

  • New FPBT package created to contain functions concerning
    the FBPT Table found within ACPI FPDT

Signed-off-by: Prince Geutler geutlerprince@gmail.com

Copy link
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

This looks really useful, thanks!

Besides my very small nit, we're going to need tests.

But this is a super useful tool and I hope you are ok with writing them.

cmds/contrib/fbptcat/fbptcat.go Outdated Show resolved Hide resolved
@rminnich
Copy link
Member

if would really be nice to have this, can you tidy it up and we'll try again?

@princegeutler
Copy link
Contributor Author

princegeutler commented Sep 20, 2023

if would really be nice to have this, can you tidy it up and we'll try again?

Hi, yes thank you for the ping.

@princegeutler
Copy link
Contributor Author

Hi,

I have a few questions:

  • I am unsure how to run the test-suite locally. Is there an easy way to achieve this?

  • I am having a bunch of commits in this pull request because I keep having to merge changes within u-root to my 'development' branch to make a new commit or amend my commits. Is this an issue? If so is there a more elegant way of modifying my pull request.

Thank you.

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Attention: 111 lines in your changes are missing coverage. Please review.

Files Coverage Δ
pkg/acpi/fpdt/fpdt.go 26.66% <26.66%> (ø)
pkg/acpi/fpdt/fbpt/fbpt.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@princegeutler
Copy link
Contributor Author

princegeutler commented Sep 27, 2023

Codecov Report

Attention: 111 lines in your changes are missing coverage. Please review.

Files Coverage Δ
pkg/acpi/fpdt/fpdt.go 26.66% <26.66%> (ø)
pkg/acpi/fbpt/fbpt.go 0.00% <0.00%> (ø)
... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Most of the code that I wrote in pkg/fpdt and pkg/fbpt parses the tables' contents which I can not verify since content can vary. I wrote a test to check the fpdt tables integrity by calculating the checksum (the fpbt table resides inside the fpdt table so its included in the checksum calculation). Are other tests/more code coverage needed?

EDIT : Just read integration test section of u-root, currently still working on tests.

@rminnich
Copy link
Member

rminnich commented Oct 1, 2023

Well, you do need to add a DCO via git commit -s --amend for each commit

@princegeutler princegeutler force-pushed the development branch 15 times, most recently from a1c4cf1 to 6d1ac04 Compare October 2, 2023 05:53
@princegeutler princegeutler force-pushed the development branch 6 times, most recently from b18068f to 91f2e08 Compare October 20, 2023 04:09
FBPT = Firmware Basic Performance Boot Table

fbptcat is a shell command for printing the raw contents of
Firmware Basic Performance Boot Table (FBPT) within ACPI FPDT

New FPDT package created to contain functions concerning
all tables and structures within ACPI FPDT

New FPBT package created to contain functions concerning
the FBPT Table found within ACPI FPDT

Signed-off-by: Prince Geutler <tun50582@temple.edu>
@rminnich rminnich added the Awaiting author Waiting for new changes or feedback for author. label Oct 20, 2023
@iangoegebuer
Copy link

iangoegebuer commented Oct 20, 2023

We have been chatting about how best to get more coverage.

At least for fbpt.go you may be able to do something similar to smbios_test.go and mock the memory ready functions. Every call to Seek and ReadFull go to a function which ultimately calls mem.Seek|ReadFull. Then you can override those functions for the test.

I'm worried refactoring to use memio might be slower due to the number of sequential reads you need to do here and the number of times memio would have to open, seek, read, close.

func TestSMBIOSBaseNotFound(t *testing.T) {
systabPath = "testdata/systab_NOT_FOUND"
defer func(old func(base int64, uintn memio.UintN) error) { memioRead = old }(memioRead)
memioRead = func(base int64, uintn memio.UintN) error {
return nil
}
_, _, err := SMBIOSBase()
want := "could not find _SM_ or _SM3_ via /dev/mem from 0x000f0000 to 0x00100000"
if err.Error() != want {
t.Errorf("SMBIOSBase(): '%v', want %s", err, want)
}
}

@rminnich rminnich added the automerge Applying this label auto-merges the PR when ready label Nov 4, 2023
@rminnich rminnich merged commit f37e65a into u-root:main Nov 4, 2023
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants