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

backend: Use Buffer<u8> instead of Buffer<u32> #677

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Nov 20, 2023

I don't think sendmsg/recvmsg are necessarily guaranteed to read/write a multiple of 4 bytes, so for correctness we need to buffer bytes rather than words. This also avoids unsafe casts.

This is based on #666. Not sure if the helpers here for reading/writing words to the buffer could be improved. Perhaps as methods of Buffer, if that's only being used for this.

This doesn't seem particularly worse to handle than the previous way, since either version has to deal with lengths in bytes vs. words, and array padding.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fdd88c4) 73.03% compared to head (474246a) 73.00%.

Files Patch % Lines
wayland-backend/src/rs/wire.rs 97.95% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   73.03%   73.00%   -0.04%     
==========================================
  Files          48       48              
  Lines        7841     7826      -15     
==========================================
- Hits         5727     5713      -14     
+ Misses       2114     2113       -1     
Flag Coverage Δ
main 58.26% <98.38%> (-0.07%) ⬇️
test-- 78.16% <93.54%> (+0.08%) ⬆️
test--server_system 61.10% <93.54%> (+0.03%) ⬆️
test-client_system- 68.96% <88.70%> (+0.05%) ⬆️
test-client_system-server_system 51.42% <0.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elinorbgr
Copy link
Member

That looks pretty clean overall, is there something you want to change, given you've set this PR as a draft?

Both are documented as wrappers around `memmove`. But this one is safe.
@ids1024 ids1024 marked this pull request as ready for review December 9, 2023 19:48
@elinorbgr elinorbgr merged commit 7e98c79 into Smithay:master Dec 11, 2023
14 of 15 checks passed
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.

2 participants