-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add Bulk transfer async API #64
Conversation
src/device_handle/async_api.rs
Outdated
pub fn new_bulk( | ||
device: &'d DeviceHandle<C>, | ||
endpoint: u8, | ||
buffer: &'b mut [u8], |
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.
If you mem::forget()
the AsyncTransfer
so its drop
never runs to cancel the transfer, this buffer may be invalidated or mutably borrowed elsewhere while libusb is still handling a transfer accessing it.
Unfortunately Pin
cannot solve this. It's the same problem that Rust APIs for io_uring run into. The transfer needs to own the buffer so that it remains valid in case drop
is not run.
src/device_handle/async_api.rs
Outdated
/// returned Err, or the callback has gotten an Err. | ||
// Step 3 of async API | ||
#[allow(unused)] | ||
pub fn submit(&mut self) -> Result<(), TransferError> { |
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.
Can you even call this with &mut self
when the constructor returns it as Pin
? If you could obtain a &mut AsyncTransfer
without unsafe
, then you can also mem::swap
it, and your use of Pin
wouldn't be doing any good in protecting its self-reference.
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.
Yeah, you're right. I put it as mut because I wasnt sure if it would be safe to have it as a shared reference. Can I just change it to &? Would I need to make it !Sync?
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.
Or perhaps it should be submit(self: &mut Pin<Box<Self>>)
?
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 think its fixed now, lmk
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.
&self
may actually be safe given the locking inside libusb_submit_transfer
, but I'd have to look more closely.
// inside the custom user data field of the transfer struct, and then call the | ||
// user provided closure from there. | ||
// Step 4 of async API | ||
extern "system" fn transfer_cb(transfer: *mut ffi::libusb_transfer) { |
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.
If the user-supplied callback panics, it's undefined behavior for it to propagate into C, so this needs a catch_unwind
.
// Safety: The pointer shouldn't be a fat pointer, and should be valid, so | ||
// this should be sound | ||
let closure = unsafe { | ||
let closure: *mut F = std::mem::transmute(transfer.user_data); | ||
&mut *closure | ||
}; |
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.
libusb_cancel_transfer
in the drop
implementation requests cancellation asynchronously, but after drop
returns, the AsyncTransfer
memory may be invalidated before this callback is guaranteed to run (or complete, if already running on another thread). The callback may even still be called with a transfer completion after cancellation if it was in the middle of completing at the time of cancellation. Therefore this reference is not guaranteed to be valid.
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 callback may even still be called with a transfer completion after cancellation if it was in the middle of completing at the time of cancellation.
Are you saying this ordering is possible:
- Submit transfer
- Drop
- Callback informs of cancellation
- Callback informs of transfer completion
I wouldn't expect 4 to ever come before 3 - are you sure that is possible? If not possible, perhaps I could actually intentionally leak the struct with Box::leak, and then get the box back inside the cancellation callback and properly drop the components?
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 callback should only get called once for a single transfer (until resubmitted), so you won't get both 3 and 4. What I'm not sure about is if you can get a LIBUSB_TRANSFER_COMPLETED
callback after calling libusb_transfer_cancel
if the transfer has already completed but the event loop has not called the callback yet, or whether this would still be LIBUSB_TRANSFER_CANCELLED
but have nonzero actual_length
.
I think deferring the drop to the callback could work, though I'd probably use ManuallyDrop
over Box::leak
to make it explicit.
match transfer.status { | ||
LIBUSB_TRANSFER_CANCELLED => { | ||
// Step 5 of async API: Transfer was cancelled, free the transfer | ||
unsafe { ffi::libusb_free_transfer(transfer) } |
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.
This is the only call to libusb_free_transfer
. If the transfer has already completed on drop
, the callback is never called with this status and the libusb_transfer is leaked.
LIBUSB_TRANSFER_COMPLETED => { | ||
debug_assert!(transfer.length >= transfer.actual_length); // sanity | ||
let data = unsafe { | ||
std::slice::from_raw_parts(transfer.buffer, transfer.actual_length as usize) |
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.
One other safety consideration is that the transfer is no longer considered busy by libusb at the time of the callback, and the AsyncTransfer might be owned by another thread that could resubmit it during the execution of this callback or the user closure. That would illegally alias and mutate the transfer
and buffer references here.
This also inspired me to finally write up some thoughts on async USB in Rust in general and an API for it I've been planning on implementing but haven't gotten to yet. Safety issues aside, callbacks are awkward in Rust due to single ownership, and would require a bunch of Posted more over at #62 (comment) to avoid hijacking your PR. |
Closing in favor of #65 |
Closes #62
Disclaimer: I am very new to unsafe rust, self-referential structs, and libusb's async API. Please vet this code.
This WIP PR adds support for bulk reads with libusb's async API. The following needs to be done before a merge: