Use sync.OnceFunc, sync.OnceValue[s]#5261
Use sync.OnceFunc, sync.OnceValue[s]#5261kolyshkin wants to merge 6 commits intoopencontainers:mainfrom
Conversation
1. Use sync.OnceValue. 2. Fix the len(buf) check -- we only need 1 byte. Real kernel output is "Y\n" so practically this change is a no-op. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
| defer func() { | ||
| intelRdtIsEnabled = intelrdt.IsEnabled | ||
| intelRdtIsCATEnabled = intelrdt.IsCATEnabled | ||
| intelRdtIsMBAEnabled = intelrdt.IsMBAEnabled | ||
| }() |
There was a problem hiding this comment.
Not a big fan of this code but could not think of a better way.
Aside from something like this maybe?
func mockEnabled(t *testing.T, rdtEnabled, catEnabled, mbaEnabled bool) {
t.Cleanup(func() {
intelRdtIsEnabled = intelrdt.IsEnabled
intelRdtIsCATEnabled = intelrdt.IsCATEnabled
intelRdtIsMBAEnabled = intelrdt.IsMBAEnabled
})
intelRdtIsEnabled = func() bool { return rdtEnabled }
intelRdtIsCATEnabled = func() bool { return catEnabled }
intelRdtIsMBAEnabled = func() bool { return mbaEnabled }
}(at least it's all mostly in one place)
| }) | ||
| return appArmorEnabled | ||
| } | ||
| var isEnabled = sync.OnceValue(func() bool { |
There was a problem hiding this comment.
one pattern I also used in some places is to have a regular function (instead of the rather ad-hoc style for these), and then wrap it in a sync.OnceValue - sometimes useful for testing (where you may not want the "once") and to keep it as a "function", not a variable that happens to be a function, i.e.;
var something = sync.OnceValue(doSomething)
func doSomething() bool {
return true
}There was a problem hiding this comment.
Yes, if the bare (unwrapped) functionality is needed for e.g. test or a benchmark.
Not the case here so I'd rather keep this as is.
|
|
||
| intelrdt := newManager(helper.config, "", helper.IntelRdtPath) | ||
| if err := intelrdt.Set(helper.config); err != nil { | ||
| intelrdt := newManager(config, "", intelRdtRoot) |
There was a problem hiding this comment.
That newManager (and the exported one) have weird signature. On that matter though; we should remove the non-exported one; it doesn't do anything;
runc/libcontainer/intelrdt/intelrdt.go
Lines 183 to 194 in eb7eaf1
There was a problem hiding this comment.
Done (as a separate patch before the actual changes).
| root = func() (string, error) { | ||
| return intelRdtRoot, nil | ||
| } |
There was a problem hiding this comment.
Do we have to restore the original in t.Cleanup ?
There was a problem hiding this comment.
We don't but we can definitely add it.
Done.
| }, | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
Maybe t.Cleanup for this? At least prevents it not being called if a t.Fatal would sneak in.
There was a problem hiding this comment.
Yeah, similar to what've self-suggested earlier in #5261 (comment)
Done (without a separate function)
...in intelrdtCheck, like all other checks already do. Best reviewed with --ignore-all-space. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is not doing anything, and tests can just instantiate the &Manager{}.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The whole struct intelRdtStatus with its methods and a sync.Once is not needed, since intelrtd.Is*Enabled methods are already run-once (or use run-once and a simple comparison). Yet it is still needed for the test to fake values returned by *Enabled. Simplify to use func pointers which a test case overwrites. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Switch from sync.Once to sync.OnceFunc and sync.OnceValues. Keep Root a function (rather than a variable) because godoc renders function doc better than a variable doc. Switch to using internal function root internally. Modify tests accordingly (and simplify NewIntelRdtTestUtil to fakeRoot). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2 participants
Switch from sync.Once to sync.OnceFunc, sync.OnceValue[s] to simplify code and reduce the number of global variables.
Do some other refactoring while at it.
Best reviewed with
--ignore-all-space.