util: include errno.h for EAGAIN [crashpad/crashpad : main]
Takuto Ikuta (Gerrit)
Takuto Ikuta voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mark Mentovai
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
Takuto, LGTM.
---
Joshua, I’m not sure that this will ever actually see `-EAGAIN`. `-EPERM`, possibly.
Most likely, `crashpad::LogOutputStream::Delegate::Log` should be made to return a `bool` instead of an `int`. I don’t think that the only working implementation of that interface that we have, in handler/linux/crash_report_exception_handler.cc, actually behaves the way that the interface description in util/stream/log_output_stream.h says it should. In particular, `__android_log_buf_write`’s [documentation](https://developer.android.com/ndk/reference/group/logging#__android_log_buf_write) and [implementation](https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/logger_write.cpp;l=385,389,395;drc=90ddfb0bd41fbb41d045f04e02476ddce200d535).
I’m also not sure why `crashpad::LogOutputStream::WriteBuffer` code seems to think it can suffer a `WriteToLog` error and then be able to call `WriteToLog` again on its failure path.
This file is giving me a weird feeling. Can you take a look and adjust if appropriate?
Related details
- Takuto Ikuta
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza added 1 comment![]( "Open in Gerrit")
Takuto, LGTM.
---
Joshua, I’m not sure that this will ever actually see `-EAGAIN`. `-EPERM`, possibly.
Most likely, `crashpad::LogOutputStream::Delegate::Log` should be made to return a `bool` instead of an `int`. I don’t think that the only working implementation of that interface that we have, in handler/linux/crash_report_exception_handler.cc, actually behaves the way that the interface description in util/stream/log_output_stream.h says it should. In particular, `__android_log_buf_write`’s [documentation](https://developer.android.com/ndk/reference/group/logging#__android_log_buf_write) and [implementation](https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/logger_write.cpp;l=385,389,395;drc=90ddfb0bd41fbb41d045f04e02476ddce200d535).
I’m also not sure why `crashpad::LogOutputStream::WriteBuffer` code seems to think it can suffer a `WriteToLog` error and then be able to call `WriteToLog` again on its failure path.
This file is giving me a weird feeling. Can you take a look and adjust if appropriate?
The Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
Related details
- Takuto Ikuta
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Joshua PerazaTakuto, LGTM.
---
Joshua, I’m not sure that this will ever actually see `-EAGAIN`. `-EPERM`, possibly.
Most likely, `crashpad::LogOutputStream::Delegate::Log` should be made to return a `bool` instead of an `int`. I don’t think that the only working implementation of that interface that we have, in handler/linux/crash_report_exception_handler.cc, actually behaves the way that the interface description in util/stream/log_output_stream.h says it should. In particular, `__android_log_buf_write`’s [documentation](https://developer.android.com/ndk/reference/group/logging#__android_log_buf_write) and [implementation](https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/logger_write.cpp;l=385,389,395;drc=90ddfb0bd41fbb41d045f04e02476ddce200d535).
I’m also not sure why `crashpad::LogOutputStream::WriteBuffer` code seems to think it can suffer a `WriteToLog` error and then be able to call `WriteToLog` again on its failure path.
This file is giving me a weird feeling. Can you take a look and adjust if appropriate?
The Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
The Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
I don't not believe you.
But:
How does that work? We don't call anything on that page, we call a different function that's not documented to return `-EAGAIN`, and (if I found the right thing) whose implementation doesn't either.
Related details
- Takuto Ikuta
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza added 1 comment![]( "Open in Gerrit")
Joshua PerazaTakuto, LGTM.
---
Joshua, I’m not sure that this will ever actually see `-EAGAIN`. `-EPERM`, possibly.
Most likely, `crashpad::LogOutputStream::Delegate::Log` should be made to return a `bool` instead of an `int`. I don’t think that the only working implementation of that interface that we have, in handler/linux/crash_report_exception_handler.cc, actually behaves the way that the interface description in util/stream/log_output_stream.h says it should. In particular, `__android_log_buf_write`’s [documentation](https://developer.android.com/ndk/reference/group/logging#__android_log_buf_write) and [implementation](https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/logger_write.cpp;l=385,389,395;drc=90ddfb0bd41fbb41d045f04e02476ddce200d535).
I’m also not sure why `crashpad::LogOutputStream::WriteBuffer` code seems to think it can suffer a `WriteToLog` error and then be able to call `WriteToLog` again on its failure path.
This file is giving me a weird feeling. Can you take a look and adjust if appropriate?
Mark MentovaiThe Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
The Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
I don't not believe you.
But:
How does that work? We don't call anything on that page, we call a different function that's not documented to return `-EAGAIN`, and (if I found the right thing) whose implementation doesn't either.
It seems \_\_android_log_buf_write() was updated to longer return -EAGAIN by this CL: https://cs.android.com/android/_/android/platform/system/logging/+/c17613c4582d4f6eecb3965bb96584f25762b827
Changes as suggested: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/6786151
Related details
- Takuto Ikuta
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takuto Ikuta (Gerrit)
Takuto Ikuta added 1 comment![]( "Open in Gerrit")
Joshua PerazaTakuto, LGTM.
---
Joshua, I’m not sure that this will ever actually see `-EAGAIN`. `-EPERM`, possibly.
Most likely, `crashpad::LogOutputStream::Delegate::Log` should be made to return a `bool` instead of an `int`. I don’t think that the only working implementation of that interface that we have, in handler/linux/crash_report_exception_handler.cc, actually behaves the way that the interface description in util/stream/log_output_stream.h says it should. In particular, `__android_log_buf_write`’s [documentation](https://developer.android.com/ndk/reference/group/logging#__android_log_buf_write) and [implementation](https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/logger_write.cpp;l=385,389,395;drc=90ddfb0bd41fbb41d045f04e02476ddce200d535).
I’m also not sure why `crashpad::LogOutputStream::WriteBuffer` code seems to think it can suffer a `WriteToLog` error and then be able to call `WriteToLog` again on its failure path.
This file is giving me a weird feeling. Can you take a look and adjust if appropriate?
Mark MentovaiThe Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
Joshua PerazaThe Errors section at the bottom of the README says EAGAIN means the logcat has been overloaded. https://cs.android.com/android/_/android/platform/system/logging/+/main:liblog/README.md
This has been reproducible in tests by logging lots of output: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2439641
I don't not believe you.
But:
How does that work? We don't call anything on that page, we call a different function that's not documented to return `-EAGAIN`, and (if I found the right thing) whose implementation doesn't either.
It seems \_\_android_log_buf_write() was updated to longer return -EAGAIN by this CL: https://cs.android.com/android/_/android/platform/system/logging/+/c17613c4582d4f6eecb3965bb96584f25762b827
Changes as suggested: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/6786151
Thanks, I abandon this and wait for your CL then.
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |