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
mds: use explicitly sized types for network and disk encoding #54519
Conversation
The size of 'unsigned' type maybe not different from different OSes. And we should always use explicitly sized type. Fixes: https://tracker.ceph.com/issues/63552 Signed-off-by: Xiubo Li <xiubli@redhat.com>
Migrator and Mutation codes too can take this change
|
and i think some what server code could also be fixed Lines 496 to 507 in 70d8c5b
|
This should be fine, because we won't do disk encoding directly with them. |
@dparmar18 I didn't see anywhere is encoding this for network or disk, did I miss something here ? |
@@ -381,7 +381,7 @@ class Capability : public Counter<Capability> { | |||
ceph_seq_t mseq = 0; | |||
|
|||
int suppress = 0; | |||
unsigned state = 0; | |||
uint32_t state = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ILP32 and LP64 architectures all implement unsigned
as unsigned int
a.k.a. uint32_t
could you point where this standard isn't being followed ?
however, I agree its always a good idea to specify the data size upfront during serialization/deserialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't tell exactly where. I just found this when I was coding the kclient patches and googled about this. It's said in some platform or OS it maybe different. As @gregsfortytwo mentioned we should always use explicitly sized type in these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is uint32_t
good enough or is __le32
more appropriate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uint32_t
is good enough here and the encode()
will help switch it to __le32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
scrolling through these methods i saw usage of fixed size types in their params hence the comment, but since they play no role in encoding, it doesn't make sense to touch them |
jenkins test make check arm64 |
The size of 'unsigned' type maybe not different from different OSes. And we should always use explicitly sized type.
Fixes: https://tracker.ceph.com/issues/63552
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
x
between the brackets:[x]
. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e