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

Return code rework #391

Merged
merged 25 commits into from
Aug 26, 2017
Merged

Return code rework #391

merged 25 commits into from
Aug 26, 2017

Conversation

vgough
Copy link
Owner

@vgough vgough commented Aug 26, 2017

This takes #380, eliminates mutation of errno, fixes formatting and fixes clang-tidy warnings.

@vgough vgough mentioned this pull request Aug 26, 2017
@vgough vgough merged commit 03d0ae7 into master Aug 26, 2017
@vgough vgough deleted the pr/380 branch August 26, 2017 06:31
Copy link
Contributor

@benrubson benrubson left a comment

Choose a reason for hiding this comment

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

Thank you for the work @vgough 👍
I just reviewed and made 2 comments below (shown by GitHub as outdated but here they are).
Thanks !

// never know...
return -eno;
}
if (writeSize == 0) {
return -EIO;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we return EIO we loose a chance to return correct errno : depending on implementations pwrite could set errno when it returns 0.
This is why I tested if (writeSize <= 0) in my initial commit.
Could we then get back to previous version here ?
Many thanks 👍

@@ -206,8 +209,6 @@ ssize_t RawFileIO::read(const IORequest &req) const {

if (readSize < 0) {
int eno = errno;
errno = 0; // just to be sure error seen in integration tests really comes
// from the function rc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have been pleased to keep this, as explained in the comment just to be sure...
Thank you 👍

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

2 participants