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

Small cleanups for copy_file sendfile fix #544

Merged
merged 5 commits into from
May 23, 2022
Merged

Small cleanups for copy_file sendfile fix #544

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edfc558
copy_file() - fix preprocessor comments in sendfile cleanup
hmoffatt May 20, 2022
3ee4ea5
copy_file() - deduplicate call to fchmod
hmoffatt May 20, 2022
e1d9a0e
check for fchmod failure
hmoffatt May 20, 2022
779ecaf
Merge remote-tracking branch 'origin/main' into sendfile-cleanup
BillyONeal May 23, 2022
8a5708a
clang-format/messages
BillyONeal May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/vcpkg/base/files.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2898,6 +2898,8 @@ namespace vcpkg
return true;
#else // ^^^ (defined(__APPLE__) // !defined(__APPLE__) vvv

destination_fd.fchmod(source_stat.st_mode, ec);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now generally before writing the output and for some paths before reading ec, instead of immediately before returning. IMO this is a behavioural change, not deduplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but is there a need to delay the fchmod until the end, requiring the duplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is really an improvement ;-)

Copy link
Contributor

@ras0219-msft ras0219-msft May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this change LGTM, but it needs to add a check for ec and an early failure if the fchmod failed. Failure to change permissions but success in copying the data doesn't sound like a successful copy to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks that's a good point, I pushed a change.


#if defined(__linux__)
// https://man7.org/linux/man-pages/man2/sendfile.2.html#NOTES
// sendfile() will transfer at most 0x7ffff000 (2,147,479,552)
Expand Down Expand Up @@ -2926,10 +2928,7 @@ namespace vcpkg
}

if (!ec)
{
destination_fd.fchmod(source_stat.st_mode, ec);
return true;
}
// Else fall back to read/write
ec.clear();
#endif // ^^^ defined(__linux__)
Expand Down Expand Up @@ -2958,7 +2957,6 @@ namespace vcpkg
}
}

destination_fd.fchmod(source_stat.st_mode, ec);
return !ec;
#endif // ^^^ !(defined(__APPLE__)
#endif // ^^^ !_WIN32
Expand Down