os/bluestore/bluestore_tool: compare retval stat() with -1#41433
Merged
tchaikov merged 3 commits intoceph:masterfrom May 23, 2021
Merged
os/bluestore/bluestore_tool: compare retval stat() with -1#41433tchaikov merged 3 commits intoceph:masterfrom
tchaikov merged 3 commits intoceph:masterfrom
Conversation
ifed01
approved these changes
May 20, 2021
Contributor
Author
|
changelog
|
tchaikov
commented
May 21, 2021
src/os/bluestore/bluestore_tool.cc
Outdated
| return {"", false}; | ||
| } | ||
| std::error_code ec; | ||
| fs::path target_path = fs::weakly_canonical(dev_target, ec); |
Contributor
Author
There was a problem hiding this comment.
-- The CXX compiler identification is GNU 7.5.0
-- The C compiler identification is GNU 7.5.0
...
/build/ceph-17.0.0-4436-g0a8ba2e2/src/os/bluestore/bluestore_tool.cc:789:34: error: 'weakly_canonical' is not a member of 'fs'
fs::path target_path = fs::weakly_canonical(dev_target, ec);
^~~~~~~~~~~~~~~~
turns out libstdc++ shipped along with GCC 7.5.0 does not offer std::filesystem::weakly_canonical() yet. and we cannot drop bionic yet. see ceph/ceph-build#1794 (comment)
before this change, stat() is always called to check if the file specified by --dev-target exists even if this option is not specified. also, we compare the retval of stat() with ENOENT, while state() returns -1 on error. after this change, stat() is called only if --dev-target is specified, and we compare the retval of stat() with -1 and 0 only, so if --dev-target option is not specified, the tool still hehaves. this change addresses a regression introduced by 94a91f5 Fixes: https://tracker.ceph.com/issues/50891 Signed-off-by: Kefu Chai <kchai@redhat.com>
Contributor
Author
|
changelog
|
ba9960a to
35c7a1b
Compare
Contributor
|
@tchaikov - wouldn't it be better at this point to split this PR into two parts:
The rationale is that part 1) is definitely worth prompt backporting along with my ceph-volume migrate feature implementation. What do you think? |
the libstdc++ shipped with GCC 7.5 does not have good support of std::filesystem, among other things, it does not offer std::filesystem::weakly_canonical(). but boost::filesystem does. and boost::filesystem is compatible with std::filesystem to some degree. so let's use it if <filesystem> is not available, we can take it as a signal that std::filesystem is not quite ready yet. Signed-off-by: Kefu Chai <kchai@redhat.com>
for better readability Signed-off-by: Kefu Chai <kchai@redhat.com>
Contributor
Author
|
@ifed01 but i do split the PR into separated commits. |
Contributor
OK, nevermind |
3 tasks
3 tasks
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: https://tracker.ceph.com/issues/50891
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox