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.

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

Fix multiple issues with POSIX AIO #715

Merged
merged 2 commits into from
Sep 4, 2017
Merged

Conversation

asomers
Copy link
Member

@asomers asomers commented Jul 31, 2017

Fixed error handling in AioCb::fsync, AioCb::read, and AioCb::write

Previously, the `AioCb`'s `in_progress` field would erroneously be set
to `true`, even if the syscall had an error

Fixes #714

AioCb::Drop will now panic for in-progress AioCb

Printing a warning message to stderr isn't really appropriate, because
there's no way to guarantee that stderr is even valid.  Nor is
aio_suspend necessarily an appropriate action to take.

CHANGELOG.md Outdated
- `AioCb::Drop` will now panic if the `AioCb` is still in-progress ([#715](https://github.com/nix-rust/nix/pull/715))

### Fixed
- Fixed error handling in `AioCb::fsync`, `AioCb::read`, and `AioCb::write` ([#715](https://github.com/nix-rust/nix/pull/715))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the specific thing that was fixed here would be good rather than having people see this and then having to reference the PR.

@Susurrus
Copy link
Contributor

Susurrus commented Aug 1, 2017

@asomers That's a more clear CHANGELOG entry, thanks. But I must ask why are these tests only ran on FreeBSD?

@Susurrus
Copy link
Contributor

Susurrus commented Aug 1, 2017

But there are still errors on Mac and a musl target that will need to be fixed.

@asomers
Copy link
Member Author

asomers commented Aug 1, 2017

It seems that on Linux, aio_write and aio_read return success even if the arguments are invalid, contrary to the man page. I don't know what's happening on OSX. Hopefully I'll know after this latest test run.

@asomers
Copy link
Member Author

asomers commented Sep 3, 2017

Somehow I seriously screwed up the squash. I'll try to fix it.

Printing a warning message to stderr isn't really appropriate, because
there's no way to guarantee that stderr is even valid.  Nor is
aio_suspend necessarily an appropriate action to take.
Previously, the `AioCb`'s `in_progress` field would erroneously be set
to `true`, even if the syscall had an error

Fixes nix-rust#714
@asomers
Copy link
Member Author

asomers commented Sep 3, 2017

bors r+ chrisvest

bors bot added a commit that referenced this pull request Sep 3, 2017
715: Fix multiple issues with POSIX AIO r=asomers a=asomers

Fixed error handling in `AioCb::fsync`, `AioCb::read`, and `AioCb::write`
    
    Previously, the `AioCb`'s `in_progress` field would erroneously be set
    to `true`, even if the syscall had an error
    
    Fixes #714

AioCb::Drop will now panic for in-progress AioCb

    Printing a warning message to stderr isn't really appropriate, because
    there's no way to guarantee that stderr is even valid.  Nor is
    aio_suspend necessarily an appropriate action to take.
@asomers
Copy link
Member Author

asomers commented Sep 4, 2017

It looks like bors is ignoring me because chrisvest doesn't have the authority to approve a merge? I thought that everyhhing following "bors r+" was ignored, but I guess not.

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 4, 2017

Not awaiting review

@bors
Copy link
Contributor

bors bot commented Sep 4, 2017

@bors bors bot merged commit bd2cda1 into nix-rust:master Sep 4, 2017
@notriddle
Copy link
Contributor

It wasn't ignoring you. It was just being slow.

@asomers asomers deleted the issue_714 branch September 4, 2017 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants