os/bluestore: fix unused function warnings in bluefs_types#55428
Closed
jamiepryde wants to merge 1 commit intoceph:mainfrom
Closed
os/bluestore: fix unused function warnings in bluefs_types#55428jamiepryde wants to merge 1 commit intoceph:mainfrom
jamiepryde wants to merge 1 commit intoceph:mainfrom
Conversation
tchaikov
reviewed
Feb 4, 2024
src/os/bluestore/bluefs_types.h
Outdated
| op_bl.claim_append(from.op_bl); | ||
| } | ||
|
|
||
| DENC_HELPERS |
Contributor
There was a problem hiding this comment.
i think the compiler warning is a signal that we are not using the DENC macro for defining the dencoders of bluefs_transaction_t. in fact, bluefs_transaction_t defines its own dencoders manually without relying on DENC. what DENC_HELPERS provides to this class is but
static void _denc_start(size_t& p, \
__u8 *struct_v, \
__u8 *struct_compat, \
char **, uint32_t *) { \
p += 2 + 4; \
} \for calculating the size of the header of the serialized bluefs_transaction_t. and DENC_FINISH() here is a noop.
so i'd suggest apply following patch
diff --git a/src/os/bluestore/bluefs_types.cc b/src/os/bluestore/bluefs_types.cc
index ff9c96ab660..a17c3bc8a3c 100644
--- a/src/os/bluestore/bluefs_types.cc
+++ b/src/os/bluestore/bluefs_types.cc
@@ -228,10 +228,10 @@ std::ostream& operator<<(std::ostream& out, const bluefs_fnode_delta_t& delta)
// bluefs_transaction_t
-DENC_HELPERS
void bluefs_transaction_t::bound_encode(size_t &s) const {
uint32_t crc = op_bl.crc32c(-1);
- DENC_START(1, 1, s);
+ // struct_v, struct_compat, and len
+ s += 2 + 4;
denc(uuid, s);
denc_varint(seq, s);
// not using bufferlist encode method, as it merely copies the bufferptr and not
@@ -242,7 +242,6 @@ void bluefs_transaction_t::bound_encode(size_t &s) const {
s += it.length();
}
denc(crc, s);
- DENC_FINISH(s);
}
void bluefs_transaction_t::encode(bufferlist& bl) constinstead of using DENC_HELPERS for a single line code.
Contributor
Contributor
There was a problem hiding this comment.
maybe the macro should qualify the helper functions with [[maybe_unused]]?
Contributor
Author
There was a problem hiding this comment.
Thank you both for reviewing, I have applied your suggested patch @tchaikov .
Fixes: https://tracker.ceph.com/issues/62850 Signed-off-by: Jamie Pryde <jamiepryde@gmail.com>
124a37f to
9fc47f8
Compare
Contributor
i cannot find it on tracker. after searching with "unittest_rbd_mirror". |
Contributor
|
jenkins test make check |
Contributor
infra issues. |
Contributor
|
jenkins test make check |
Contributor
|
flaky test, see https://tracker.ceph.com/issues/62934 |
Contributor
|
jenkins test make check |
Contributor
|
yet another flaky test: |
Contributor
|
jenkins test make check |
Contributor
|
jenkins test api |
aclamk
approved these changes
Feb 13, 2024
Contributor
Author
|
jenkins test windows |
Contributor
Author
|
@tchaikov Hi, can we merge this change now? Thanks |
Contributor
better off running it against the teuthology tests |
aclamk
reviewed
Feb 27, 2024
| uint32_t crc = op_bl.crc32c(-1); | ||
| DENC_START(1, 1, s); | ||
| // struct_v, struct_compat, and len | ||
| s += 2 + 4; |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
Contributor
|
@jamiepryde pls rebase and merge |
Contributor
Author
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
7 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/62850
I have run
ninja osdto confirm that osd builds, and that the compiler warnings are no longer present.Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.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 dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e