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.

Sign up for GitHub

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

Jump to bottom

refact(log): Use a replaceable logger #331

Merged
merged 1 commit into from
Apr 27, 2021
Merged

refact(log): Use a replaceable logger #331

merged 1 commit into from
Apr 27, 2021

Conversation

xaionaro
Copy link
Member

@xaionaro xaionaro commented Apr 23, 2021 •
edited
Loading

Previously it was impossible to mute fiano without affecting the global logger "log". Now it is possible.

ITS: #330

Test Plan

unit-tests

xaionaro@void:~/go/src/github.com/linuxboot/fiano$ go test ./...
ok  	github.com/linuxboot/fiano/cmds/fmap	0.065s
ok  	github.com/linuxboot/fiano/cmds/fspinfo	0.009s
?   	github.com/linuxboot/fiano/cmds/glzma	[no test files]
?   	github.com/linuxboot/fiano/cmds/guid2english	[no test files]
?   	github.com/linuxboot/fiano/cmds/utk	[no test files]
ok  	github.com/linuxboot/fiano/integration	4.650s
ok  	github.com/linuxboot/fiano/pkg/compression	6.259s
ok  	github.com/linuxboot/fiano/pkg/fmap	(cached)
ok  	github.com/linuxboot/fiano/pkg/fsp	0.012s
ok  	github.com/linuxboot/fiano/pkg/guid	0.013s
ok  	github.com/linuxboot/fiano/pkg/guid2english	0.025s
?   	github.com/linuxboot/fiano/pkg/knownguids	[no test files]
?   	github.com/linuxboot/fiano/pkg/log	[no test files]
ok  	github.com/linuxboot/fiano/pkg/uefi	0.027s
?   	github.com/linuxboot/fiano/pkg/unicode	[no test files]
?   	github.com/linuxboot/fiano/pkg/utk	[no test files]
ok  	github.com/linuxboot/fiano/pkg/visitors	7.303s
?   	github.com/linuxboot/fiano/scripts/checklicenses	[no test files]
?   	github.com/linuxboot/fiano/scripts/namecollect	[no test files]

end2end

before

xaionaro@void:~/go/src/github.com/9elements/converged-security-suite/pkg/tools$ go test ./... -run=TestCalcImageOffsetNoLogGarbage -count=1
--- FAIL: TestCalcImageOffsetNoLogGarbage (3.84s)
panic: PANIC [recovered]
	panic: PANIC

goroutine 19 [running]:
testing.tRunner.func1.2(0x6236a0, 0x6c3328)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/testing/testing.go:1144 +0x332
testing.tRunner.func1(0xc000082900)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/testing/testing.go:1147 +0x4b6
panic(0x6236a0, 0x6c3328)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/runtime/panic.go:965 +0x1b9
github.com/9elements/converged-security-suite/v2/pkg/tools.panicWriter.Write(...)
	/home/xaionaro/go/src/github.com/9elements/converged-security-suite/pkg/tools/ifd_test.go:16
log.(*Logger).Output(0xc0000ac0a0, 0x2, 0xc004d04000, 0x107, 0x0, 0x0)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/log/log.go:184 +0x284
log.Printf(0x68679e, 0x2a, 0xc0048fbcb0, 0x1, 0x1)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/log/log.go:323 +0x85
github.com/linuxboot/fiano/pkg/uefi.NewMERegion(0xc00e8b9000, 0x2fe7000, 0x40ad000, 0xc00001e2c8, 0x1, 0xc0000a80e0, 0x0, 0x1, 0x0)
	/home/xaionaro/go/pkg/mod/github.com/linuxboot/[email protected]+incompatible/pkg/uefi/meregion.go:195 +0x245
github.com/linuxboot/fiano/pkg/uefi.NewFlashImage(0xc00e8b8000, 0x4000000, 0x40ae000, 0x14, 0x0, 0x0)
	/home/xaionaro/go/pkg/mod/github.com/linuxboot/[email protected]+incompatible/pkg/uefi/flash.go:276 +0x562
github.com/9elements/converged-security-suite/v2/pkg/tools.GetRegion(0xc00e8b8000, 0x4000000, 0x40ae000, 0x0, 0x0, 0x0, 0x692401)
	/home/xaionaro/go/src/github.com/9elements/converged-security-suite/pkg/tools/ifd.go:25 +0x105
github.com/9elements/converged-security-suite/v2/pkg/tools.CalcImageOffset(0xc00e8b8000, 0x4000000, 0x40ae000, 0x1, 0x0, 0x0, 0x0)
	/home/xaionaro/go/src/github.com/9elements/converged-security-suite/pkg/tools/ifd.go:13 +0x4c
github.com/9elements/converged-security-suite/v2/pkg/tools.TestCalcImageOffsetNoLogGarbage(0xc000082900)
	/home/xaionaro/go/src/github.com/9elements/converged-security-suite/pkg/tools/ifd_test.go:27 +0xde
testing.tRunner(0xc000082900, 0x691c18)
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/testing/testing.go:1194 +0xef
created by testing.(*T).Run
	/home/xaionaro/.gimme/versions/go1.16.linux.amd64/src/testing/testing.go:1239 +0x2b3
FAIL	github.com/9elements/converged-security-suite/v2/pkg/tools	3.850s
FAIL

after

xaionaro@void:~/go/src/github.com/9elements/converged-security-suite/pkg/tools$ vi ../../go.mod
xaionaro@void:~/go/src/github.com/9elements/converged-security-suite/pkg/tools$ go test ./... -run=TestCalcImageOffsetNoLogGarbage -count=1
go: github.com/linuxboot/[email protected]+incompatible: missing go.sum entry; to add it:
	go mod download github.com/linuxboot/fiano
xaionaro@void:~/go/src/github.com/9elements/converged-security-suite/pkg/tools$ go mod download github.com/linuxboot/fiano
xaionaro@void:~/go/src/github.com/9elements/converged-security-suite/pkg/tools$ go test ./... -run=TestCalcImageOffsetNoLogGarbage -count=1
ok  	github.com/9elements/converged-security-suite/v2/pkg/tools	2.437s

@xaionaro xaionaro requested a review from rjoleary April 23, 2021 20:00
@xaionaro xaionaro force-pushed the refactor/replaceable_logger branch 2 times, most recently from 787591f to d6d253a Compare April 23, 2021 20:03
This was referenced Apr 23, 2021
Closed
Merged
@codecov-commenter
Copy link

Codecov Report

Merging #331 (d6d253a) into master (91b79e9) will not change coverage.
The diff coverage is 4.76%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #331   +/-   ##
=======================================
  Coverage   36.74%   36.74%           
=======================================
  Files          47       47           
  Lines        3132     3132           
=======================================
  Hits         1151     1151           
  Misses       1761     1761           
  Partials      220      220
Impacted Files Coverage Δ
cmds/fmap/fmap.go 0.00% <0.00%> (ø)
cmds/fspinfo/main.go 0.00% <0.00%> (ø)
pkg/guid/guid.go 82.85% <0.00%> (ø)
pkg/guid2english/transformer.go 91.48% <0.00%> (ø)
pkg/uefi/file.go 17.03% <0.00%> (ø)
pkg/uefi/firmwarevolume.go 37.93% <0.00%> (ø)
pkg/uefi/meregion.go 10.76% <0.00%> (ø)
pkg/uefi/section.go 25.64% <0.00%> (ø)
pkg/visitors/assemble.go 55.00% <0.00%> (ø)
pkg/visitors/find.go 52.13% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91b79e9...d6d253a. Read the comment docs.

GanShun
GanShun previously approved these changes Apr 23, 2021
View reviewed changes
pkg/log/logger.go Outdated
}

// Fatalf logs a fatal message and immediately executes the application
// with os.Exit.
Copy link
Member

Choose a reason for hiding this comment

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

Specifically in the case of the default logger, this relies on log.Fatalf exiting right? so it theoretically is possible for you to swap a new logger in that doesn't exit. Probably just clarify it in the comments, I doubt this will be an issue anyone will actually face.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@xaionaro
refact(log): Use a replaceable logger
86d0011 
Previously it was impossible to mute fiano without
affecting the global logger "log". Now it is possible.

Signed-off-by: Dmitrii Okunev <[email protected]>
@xaionaro xaionaro force-pushed the refactor/replaceable_logger branch from d6d253a to 86d0011 Compare April 23, 2021 20:29
@xaionaro xaionaro merged commit 991eadf into master Apr 27, 2021
@xaionaro xaionaro mentioned this pull request Apr 27, 2021
Closed
xaionaro added a commit that referenced this pull request Dec 21, 2021
@xaionaro
refact(log): Use a replaceable logger (#331)
87f93e8 
Previously it was impossible to mute fiano without affecting the global logger "log". Now it is possible.

Signed-off-by: Dmitrii Okunev <[email protected]>
@xaionaro xaionaro deleted the refactor/replaceable_logger branch December 21, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GanShun GanShun GanShun approved these changes

@rjoleary rjoleary Awaiting requested review from rjoleary

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xaionaro @codecov-commenter @GanShun