Skip to content

Properly use result value for dump IPC cmd - #60726

Merged
josalem merged 2 commits into
dotnet:mainfrom
josalem:dev/josalem/fix-dump-errorcode
Oct 27, 2021
Merged

Properly use result value for dump IPC cmd#60726
josalem merged 2 commits into
dotnet:mainfrom
josalem:dev/josalem/fix-dump-errorcode

Conversation

@josalem

@josalemjosalem commented Oct 21, 2021

Copy link
Copy Markdown
Contributor

@lateralusX caught a small error in the dump IPC command error handling while doing some refactoring. Pulling out the fix for that observation into a separate PR for easier backporting.

Currently, the dump IPC command will always return "false" even if the dump is successful. This change fixes up the error handling to use the actual error code from creating the dump.

CC @tommcdon

@josalemjosalem added this to the 7.0.0 milestone Oct 21, 2021
@josalem
josalem requested review from a team and lateralusXOctober 21, 2021 18:33
@josalemjosalem self-assigned this Oct 21, 2021
@ghost

ghost commented Oct 21, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

@lateralusX caught a small error in the dump IPC command error handling while doing some refactoring. Pulling out the fix for that observation into a separate PR for easier backporting.

Currently, the dump IPC command will always return "false" even if the dump is successful. This change fixes up the error handling to use the actual error code from creating the dump.

CC @tommcdon

Author:josalem
Assignees:josalem
Labels:

area-Diagnostics-coreclr

Milestone:7.0.0

@josalemjosalem mentioned this pull request Oct 21, 2021
Merged
am11
am11 reviewed Oct 21, 2021
View reviewed changes
Comment threadsrc/native/eventpipe/ds-dump-protocol.c Outdated
@runfoapprunfoappBot mentioned this pull request Oct 21, 2021
Closed
@lateralusX

lateralusX commented Oct 25, 2021•
edited
Loading

Copy link
Copy Markdown
Member

All creds finding this issue goes to @am11 and @lambdageek, thanks for bringing it to my attention.

@josalem

josalem commented Oct 25, 2021

Copy link
Copy Markdown
ContributorAuthor

The remaining test failures appear to be #60705. I'll merge today

@josalem
josalem merged commit b0b0b14 into dotnet:mainOct 27, 2021
@josalem
josalem deleted the dev/josalem/fix-dump-errorcode branch October 27, 2021 00:10
@ghostghost locked as resolved and limited conversation to collaborators Nov 26, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lateralusXlateralusXlateralusX approved these changes
+2 more reviewers
@am11am11am11 left review comments
@mikem8361mikem8361mikem8361 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclr

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@josalem@lateralusX@am11@mikem8361