-
Notifications
You must be signed in to change notification settings - Fork 360
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
Interpret windows exit codes as a signed integer #18282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit if you have a sec, I'll re-approve quickly. No big deal if not.
@@ -128,7 +128,7 @@ func (ds *Datastore) SetHostScriptExecutionResult(ctx context.Context, result *f | |||
res, err := tx.ExecContext(ctx, updStmt, | |||
output, | |||
result.Runtime, | |||
result.ExitCode, | |||
int32(result.ExitCode), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but I don't think this is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that it might be just for the test case (otherwise the cast to signed happens in fleetd, not server-side). That's fine to leave it there but if you have a sec to add a comment as for the why of this, as it's the kind of thing someone may see and have a reflex to remove as unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be very happy to put a comment 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the server as well because I wanted to make sure the bug gets fixed for clients where orbit can't be updated for some reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer I remove the server-side overflow check and just rely on the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, your rationale makes sense to me!
#17695 The windows exit code is a 32-bit unsigned integer, but the command interpreter treats it like a signed integer. When a process is killed, it returns 0xFFFFFFFF (interpreted as -1). We convert the integer to an signed 32-bit integer to flip it to a -1 to match our expectations, and fit in our db column. https://en.wikipedia.org/wiki/Exit_status#Windows FIxed on both the client and server side.
#17695
The windows exit code is a 32-bit unsigned integer, but the command interpreter treats it like a signed integer. When a process is killed, it returns 0xFFFFFFFF (interpreted as -1). We convert the integer to an signed 32-bit integer to flip it to a -1 to match our expectations, and fit in our db column.
https://en.wikipedia.org/wiki/Exit_status#Windows
FIxed on both the client and server side.
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.See Changes files for more information.