cmd/containerd: use golang.org/x/sys/windows.SetStdHandle()#7511
cmd/containerd: use golang.org/x/sys/windows.SetStdHandle()#7511kzys merged 1 commit intocontainerd:mainfrom
Conversation
|
|
||
| kernel32 = windows.NewLazySystemDLL("kernel32.dll") | ||
| setStdHandle = kernel32.NewProc("SetStdHandle") | ||
| allocConsole = kernel32.NewProc("AllocConsole") |
There was a problem hiding this comment.
Wondering if we should upstream this one to golang.org/x/sys ?
| if st, err := panicFile.Stat(); err == nil { | ||
| if st.Size() == 0 { | ||
| sh := uint32(windows.STD_ERROR_HANDLE) | ||
| setStdHandle.Call(uintptr(sh), uintptr(oldStderr)) | ||
| windows.SetStdHandle(sh, oldStderr) | ||
| panicFile.Close() | ||
| os.Remove(panicFile.Name()) |
There was a problem hiding this comment.
I'm still trying to get my head around this logic;
- why do we only close
panicFile(panic.log) if it's empty? (could this be the cause of some "file already in use by another process" errors in CI? - why do we only restore StdErr if
panic.logis empty?
If anyone has any ideas; happy to open a PR to change this
There was a problem hiding this comment.
I agree this logic seems weird. However, I didn't look through commit history for a reason why it exists this way. It does seem like we could be leaking the file handle though.
There was a problem hiding this comment.
No, not a lot of history available; most of this was added by John in moby/moby@57aef3b#diff-96f1675ac02e434ec94a83543bea56ea711c396b10a1457997edf21194b6d343 (that was 7 Years ago, so I doubt he'd still know), with only a single change after that in moby/moby@33a779e
(heh, looks like on that one we could've caught that we could've dropped the intermediate variable to avoid the type-casting)
There was a problem hiding this comment.
I think this is ok right? We only hit this when shutting down, and we only remove when the panic file is empty. This just avoids the need on startup to move it. But ironically we check on the Open|Append that its of size 0 anyways so I think this is mostly wasted. But its not a handle leak since the process is exiting anyways. Although it would be "better" to always call close on it yes.
There was a problem hiding this comment.
Thanks for looking; yes I agree if we only call this on shutdown, the missing close is not an issue, other than the logic in this code being quite confusing 😂 (so I might do a follow-up to cleanup, but definitely not in this PR)
thaJeztah
force-pushed
the
use_SetStdHandle
branch
from
30bd49d to
cc1d212
Compare
thaJeztah
force-pushed
the
use_SetStdHandle
branch
from
cc1d212 to
66714b1
Compare
|
@estesp (saw you were online); could you perhaps give CI a kick? I think the failure is unrelated. |
golang.org/x/sys/windows now implements this, so we can use that instead of a local implementation. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
use_SetStdHandle
branch
from
66714b1 to
752bff9
Compare
|
Restarted the failed CircleCI tests. |
|
Looks like CI is still a bit wonky |
|
Failures restarted; hopefully will get to green this time! |
|
It is green! " 44 successful checks". |
6 participants
golang.org/x/sys/windows now implements this, so we can use that instead of a local implementation.