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

Win32Error: Add to_s and inspect methods #3331

Merged
merged 1 commit into from
Apr 14, 2021
Merged

Win32Error: Add to_s and inspect methods #3331

merged 1 commit into from
Apr 14, 2021

Conversation

ashie
Copy link
Member

@ashie ashie commented Apr 14, 2021

Which issue(s) this PR fixes:
none

What this PR does / why we need it:
Additional fix for #3325 and #3329.
Since the previous code doesn't have to_s method, it shows only Fluent::Win32Error, doesn't show the detail.

Docs Changes:
none

Release Note:
Same with #3325

@ashie ashie requested review from cosmo0920 and kenhys April 14, 2021 05:14
kenhys
kenhys approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@kenhys kenhys left a comment

Choose a reason for hiding this comment

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

I'm not sure #to_s and #message should be implemented separately.
Anyway this change LGTM.

@ashie
Win32Error: Add to_s and inspect methods and remove message
9a2be48 
Since the previous code doesn't have to_s method, it shows only
`Fluent::Win32Error`, doesn't show the detail.

Signed-off-by: Takuro Ashie <[email protected]>
@ashie ashie force-pushed the followup-#3325-2 branch from d4cdea6 to 9a2be48 Compare April 14, 2021 06:06
@ashie
Copy link
Member Author

ashie commented Apr 14, 2021

I'm not sure #to_s and #message should be implemented separately.

Ah sorry, I already removed #message locally but forgot to push it.

cosmo0920
cosmo0920 approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

Looks reasonable for me. 👍

@ashie
Copy link
Member Author

ashie commented Apr 14, 2021

Thanks.

BTW I found a weird behavior of td-agent 4.x on Windows by this patch.
Sometimes WindowsFile returns a weird return code on calling CreateFile against a non-existent file.
Sometimes ERROR_FILE_NOT_FOUND (158) or sometimes ERROR_SHARING_VIOLATION (32).
Probably Ruby's internal code resets the error code.
We may have to add a method to get correct error code to win32-api, like Fiddle::win32_last_error or FFI::LastError.winapi_error

@ashie ashie merged commit 72267d8 into fluent:master Apr 14, 2021
@ashie ashie deleted the followup-#3325-2 branch April 14, 2021 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cosmo0920 cosmo0920 cosmo0920 approved these changes

@kenhys kenhys kenhys approved these changes

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
@ashie @kenhys @cosmo0920