Skip to content

Fix #306 #307

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

Merged
merged 2 commits into from
Mar 21, 2022
Merged

Fix #306 #307

merged 2 commits into from
Mar 21, 2022

Conversation

taigrr
Copy link
Contributor

@taigrr taigrr commented Mar 19, 2022

Please see #306
The HEAD of main already fails existing test cases, and this patch causes those test cases to pass.

tai@juggernaut:~/code/foss/ghw/pkg/snapshot[jaypipes ?]$ go test
--- FAIL: TestCloneSystemTree (0.07s)
    clonetree_linux_test.go:85: Expected nil err, but got open /sys/devices/system/node/node0/hugepages/hugepages-1048576kB/demote: permission denied
FAIL
exit status 1
FAIL    github.com/jaypipes/ghw/pkg/snapshot    0.076s
tai@juggernaut:~/code/foss/ghw/pkg/snapshot[jaypipes ?]$ git co hugepages-demote
Switched to branch 'hugepages-demote'
Your branch is up to date with 'origin/hugepages-demote'.
tai@juggernaut:~/code/foss/ghw/pkg/snapshot[hugepages-demote ?]$ go test
PASS
ok      github.com/jaypipes/ghw/pkg/snapshot    0.100s
tai@juggernaut:~/code/foss/ghw/pkg/snapshot[hugepages-demote ?]$

@taigrr
Copy link
Contributor Author

taigrr commented Mar 19, 2022

Looks like the project is using go <= 1.15. I'll update the code to be more backwards compatible.

taigrr added 2 commits March 19, 2022 13:06

Verified

This commit was signed with the committer’s verified signature.
taigrr Tai Groot
Signed-off-by: Tai Groot <tai@taigrr.com>

Verified

This commit was signed with the committer’s verified signature.
taigrr Tai Groot
Signed-off-by: Tai Groot <tai@taigrr.com>
@taigrr taigrr force-pushed the hugepages-demote branch from 525c485 to f031448 Compare March 19, 2022 20:06
@ffromani
Copy link
Collaborator

the proper fix would be to NOT copy the write-only files, we had some examples of this already. But we can improve later, this seems like a good enough fix. Thanks!

@ffromani ffromani merged commit c82d7b1 into jaypipes:main Mar 21, 2022
@taigrr
Copy link
Contributor Author

taigrr commented Mar 21, 2022

Aha, I referenced the code here:

buf, err := ioutil.ReadFile(fp)
if err != nil {
if errors.Is(err, os.ErrPermission) {
// example: /sys/devices/virtual/block/zram0/compact is 0400
trace("permission denied reading %q - skipped\n", fp)
continue
}
return err
}

@ffromani
Copy link
Collaborator

Aha, I referenced the code here:

buf, err := ioutil.ReadFile(fp)
if err != nil {
if errors.Is(err, os.ErrPermission) {
// example: /sys/devices/virtual/block/zram0/compact is 0400
trace("permission denied reading %q - skipped\n", fp)

sure, again this fix is good! we can perhaps improve even more, but this can wait a bit. Better to have a working ghw-snapshot sooner than later.

If I get some more ghw time this week, I'll perhaps post a further refinement.

@jaypipes
Copy link
Owner

@taigrr hey, sorry for the late response here, just wanted to say thank you for the bug report and the fix! :)

@taigrr
Copy link
Contributor Author

taigrr commented Mar 24, 2022

@jaypipes no worries, we got there :)

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

3 participants