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

Implement Clone for descriptor types and/or expose raw libusb_*_descriptor fields and allow public construction from them #132

Open
Qyriad opened this issue May 10, 2022 · 3 comments

Comments

@Qyriad
Copy link

Qyriad commented May 10, 2022

Types like libusb_interface_descriptor and libusb_endpoint_descriptor are very close to PODs, only barely not due to pointers for the subordinate descriptors, which are simply bytes slices, making a deep copy only being a byte-wise copy of the slice, and then all the other fields of those descriptors. Meanwhile libusb_device_descriptor is outright unconditionally a POD struct. Yet these raw types and their Rust wrappers like DeviceDescriptor do not implement Clone (or any custom equivalent), and thus there is no way to obtain this data outside of libusb's allocated memory without using something like std::mem::transmute_copy() or making an additional control_read() to re-request these descriptors.

The only problem I see is that ConfigurationDescriptor implements Drop in order to call libusb_free_config_descriptor(). How much of a problem this is possibly a matter of opinion: libusb_free_config_descriptor() effectively just calls free() for all involved data structures, so running libusb_free_config_descriptor() on a manually-allocated configuration descriptor should be completely safe, although it does feel at least a little cursed. If you do consider this a problem, however, I don't believe it is an unsolvable one — some way to manually free the cloned data (presumably with std::alloc::dealloc()) or to use the default implementation of Drop only on the cloned data is perfectly feasible.

Would you be open to allowing cloning descriptor data in some form like this? If so, I am willing to implement it myself and open a PR.

@a1ien
Copy link
Owner

a1ien commented May 10, 2022

so running libusb_free_config_descriptor() on a manually-allocated configuration descriptor should be completely safe, although it does feel at least a little cursed

No it's not safe. If in application we use custom allocator we have big problem.

About other questions I need to think a little

@a1ien
Copy link
Owner

a1ien commented May 10, 2022

I took a closer look, and I think not need adding any Clone derive for descriptor.
If you want Clone the ConfigurationDescriptor you can easy wrap it to Arc or Rc. We can add ConfigurationDescriptorInner
and write something like this

struct ConfigurationDescriptorInner {
    descriptor: *const libusb_config_descriptor,
}

/// Describes a configuration.
pub struct ConfigDescriptor {
    inner: Arc<ConfigurationDescriptorInner>,
}

impl Drop for ConfigurationDescriptorInner {
    fn drop(&mut self) {
        unsafe {
            libusb_free_config_descriptor(self.descriptor);
        }
    }
}

But I think better doing this in application that use rusb.
And other descriptors(Interface and endpoint) require ConfigurationDescriptor be alive.
For DeviceDescriptor you can wrap it also to Arc or Rc

@Qyriad
Copy link
Author

Qyriad commented Dec 13, 2022

I've been thinking about this: would you be open to an unsafe fn as_raw method on descriptor types to get a pointer to (or a copy of) the raw libusb1-sys type, and then also provide an unsafe fn from_raw constructor to construct the rusb types from the libusb1-sys types? Many standard library types offer similar APIs, such as Box, Vec, and File, and then handling dropping correctly is up to the caller of the unsafe functions.

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

No branches or pull requests

2 participants