block: Using feature bits to check the read-only flag #7294
Conversation
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
rbradford
reviewed
Aug 25, 2025
virtio-devices/src/block.rs
Outdated
| } | ||
|
|
||
| fn read_only(&self) -> bool { | ||
| (self.features() & (1u64 << VIRTIO_BLK_F_RO)) != 0 |
Member
There was a problem hiding this comment.
The code above uses has_feature() but this does it manually - any reason for that?
Contributor
Author
There was a problem hiding this comment.
Nothing. I just feel a little bit strange to include a function declaration in this file of these structs' definitions.
rbradford
reviewed
Aug 25, 2025
virtio-devices/src/block.rs
Outdated
|
|
||
| impl BlockEpollHandler { | ||
| fn check_request(features: u64, request_type: RequestType) -> result::Result<(), ExecuteError> { | ||
| let has_feature = |feature_pos: u64| -> bool { (features & (1u64 << feature_pos)) != 0 }; |
Member
There was a problem hiding this comment.
Extract this to a common piece of code and and then use this for read_only ?
lisongqian
force-pushed
the
read-only
branch
2 times, most recently
from
f284a34 to
d7d97ff
Compare
phip1611
reviewed
Aug 26, 2025
block/src/lib.rs
Outdated
| Read(#[source] GuestMemoryError), | ||
| #[error("Failed to read_exact")] | ||
| ReadExact(#[source] io::Error), | ||
| #[error("Can't execute an operation other than `read` on a read-only device.")] |
Member
There was a problem hiding this comment.
to align style with other error messages:
Suggested change
| #[error("Can't execute an operation other than `read` on a read-only device.")] | |
| #[error("Can't execute an operation other than `read` on a read-only device")] |
This patch changes the read-only check using acked features bit, which will help to check more features. Signed-off-by: Songqian Li <[email protected]>
auto-merge was automatically disabled
August 26, 2025 08:56
Head branch was pushed to by a user without write access
phip1611
reviewed
Aug 26, 2025
| } | ||
|
|
||
| fn has_feature(features: u64, feature_flag: u64) -> bool { | ||
| (features & (1u64 << feature_flag)) != 0 |
Member
There was a problem hiding this comment.
nit:
Suggested change
| (features & (1u64 << feature_flag)) != 0 | |
| (features & (1 << feature_flag)) != 0 |
You can leave it as it, but this makes things harder to read than necessary
Contributor
Author
There was a problem hiding this comment.
Thank you for your review. Align style with others, so remains unchanged.
BTW, you can make multiple comments at once instead of making one at a time, which will save a lot of time. :)
Member
There was a problem hiding this comment.
BTW, you can make multiple comments at once instead of making one at a time, which will save a lot of time. :)
Thanks! I'm used to the one-comment-at-a-time approach. I will try to put more focus on the review feature to submit all my comments in a batch in the future!
phip1611
approved these changes
Aug 26, 2025
rbradford
approved these changes
Aug 26, 2025
Merged
via the queue into
cloud-hypervisor:main
with commit Aug 26, 2025
92370e8
39 of 40 checks passed
cbrewster
added a commit
to cbrewster/cloud-hypervisor
that referenced
this pull request
Dec 6, 2025
cloud-hypervisor#7294 adjusted the checks for read-only requests made to virtio-blk devices and started rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform any writes and are needed in order to access device serials from within the guest.
cbrewster
added a commit
to cbrewster/cloud-hypervisor
that referenced
this pull request
Dec 6, 2025
cloud-hypervisor#7294 adjusted the checks for read-only requests made to virtio-blk devices and started rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform any writes and are needed in order to access device serials from within the guest.
cbrewster
added a commit
to cbrewster/cloud-hypervisor
that referenced
this pull request
Dec 6, 2025
cloud-hypervisor#7294 adjusted the checks for read-only requests made to virtio-blk devices and started rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform any writes and are needed in order to access device serials from within the guest.
cbrewster
added a commit
to cbrewster/cloud-hypervisor
that referenced
this pull request
Dec 8, 2025
cloud-hypervisor#7294 adjusted the checks for read-only requests made to virtio-blk devices and started rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform any writes and are needed in order to access device serials from within the guest. Signed-off-by: Connor Brewster <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 9, 2025
#7294 adjusted the checks for read-only requests made to virtio-blk devices and started rejecting VIRTIO_BLK_T_GET_ID requests. These requests do not perform any writes and are needed in order to access device serials from within the guest. Signed-off-by: Connor Brewster <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
block: Using feature bits to check the read-only flag
This patch changes the read-only check using acked features bit, which will help to check more features.
Signed-off-by: Songqian Li [email protected]