Implement device-mapper and bio for a simple dm_sworndisk module #19
Conversation
cqs21
force-pushed
the
dev-dm
branch
2 times, most recently
from
518d874 to
9b2ef35
Compare
|
|
||
| void helper_bio_get(struct bio *bio) | ||
| { | ||
| bio_get(bio); |
There was a problem hiding this comment.
Is the indentation here four spaces? Seems a little longer than rust codes.
There was a problem hiding this comment.
The C kernel code uses tabs for indentation.
| pub unsafe fn from_raw(ptr: *mut bindings::bio) -> Self { | ||
| // Get a reference to a `bio`, so it won't disappear. And `bio_put` is | ||
| // called when it's been dropped. | ||
| bindings::bio_get(ptr); |
There was a problem hiding this comment.
Could the pointer be null? Seems not have any error handling here.
There was a problem hiding this comment.
This unsafe method assumes that users adhere to safety requirements.
|
|
||
| let start_sector = region.start * BLOCK_SECTORS; | ||
| let nr_bytes = region.len() * BLOCK_SIZE; | ||
| if nr_bytes != buf_len { |
There was a problem hiding this comment.
Why shouldn't this check be put at the front of the function before bio_alloc_bioset ?
There was a problem hiding this comment.
In order to avoid iterating the io_buffer twice.
| } | ||
|
|
||
| // Clones a bio that shares the original bio's I/O buffer. | ||
| pub fn alloc_clone(&self) -> Result<Self> { |
There was a problem hiding this comment.
The name alloc_clone with a &self which originated from C version might be misunderstood.
Try to be more descriptive like clone_with_shared_buffer().
There was a problem hiding this comment.
Since bio_alloc_clone function is an existing C concept, choosing a name as close as reasonably possible to the C side should avoid confusion and improve readability when switching back and forth between the C and Rust sides.
| } | ||
|
|
||
| /// A struct to carry the data for `bi_end_io`. | ||
| struct CallbackItem<T> { |
There was a problem hiding this comment.
Is the name BioEndFn, BioCompleteFn, OnCompleteFn more descriptive and readable?
|
|
||
| impl TargetType { | ||
| /// Provide an in-place constructor to register a new device mapper target. | ||
| pub fn register<T: TargetOperations>( |
There was a problem hiding this comment.
I wonder how can we ensure dm_register_target and dm_unregister_target appear in pairs and are non-repeating?
There was a problem hiding this comment.
If dm_register_target returns error, there is no need to call dm_unregister_target, and uninit memory will be freed by pin_init macros. Or else, dm_unregister_target will be called, when the TargetType is going to drop.
| (*target).version = version; | ||
| (*target).features = features; | ||
|
|
||
| binding_target_operations!( |
There was a problem hiding this comment.
This macro needs a clear explanation.
| /// and can be accessed by the `private` method. | ||
| pub struct Target<T: TargetOperations + Sized> { | ||
| opaque: Opaque<bindings::dm_target>, | ||
| _p: PhantomData<*mut T::Private>, |
There was a problem hiding this comment.
The PhantomData used here deserves a comment.
| region: Range<BlockId>, | ||
| } | ||
|
|
||
| unsafe impl Send for RawDisk {} |
There was a problem hiding this comment.
Missing SAFETY comment here.
| let handler = spawn(move || { | ||
| ReqQueue::handler(queue_cloned); | ||
| }); | ||
|
|
There was a problem hiding this comment.
handler() is not a appropriate function name and a noun. Maybe wrap L303-L305 to spawn_req_handler() is more readable.
No description provided.