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

device: Use OwnedFd type and bump MSRV to 1.66 #75

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ edition = "2018"
license = "MIT"
readme = "README.md"
repository= "https://github.com/raymanfx/libv4l-rs"
rust-version = "1.66"

[dependencies]
bitflags = "1.2.1"
Expand Down
81 changes: 48 additions & 33 deletions src/device.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::convert::TryFrom;
use std::path::Path;
use std::{
convert::TryFrom,
io, mem,
os::fd::{AsRawFd, RawFd},
os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd},
path::Path,
sync::Arc,
};

Expand Down Expand Up @@ -72,7 +72,7 @@ impl Device {
unsafe {
let mut v4l2_caps: v4l2_capability = mem::zeroed();
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_QUERYCAP,
&mut v4l2_caps as *mut _ as *mut std::os::raw::c_void,
)?;
Expand All @@ -91,7 +91,7 @@ impl Device {
v4l2_ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
v4l2_ctrl.id |= V4L2_CTRL_FLAG_NEXT_COMPOUND;
match v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_QUERY_EXT_CTRL,
&mut v4l2_ctrl as *mut _ as *mut std::os::raw::c_void,
) {
Expand All @@ -114,7 +114,7 @@ impl Device {
..mem::zeroed()
};
let res = v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_QUERYMENU,
&mut v4l2_menu as *mut _ as *mut std::os::raw::c_void,
);
Expand Down Expand Up @@ -169,7 +169,7 @@ impl Device {
..mem::zeroed()
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_QUERY_EXT_CTRL,
&mut queryctrl as *mut _ as *mut std::os::raw::c_void,
)?;
Expand All @@ -188,7 +188,7 @@ impl Device {
..mem::zeroed()
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_G_EXT_CTRLS,
&mut v4l2_ctrls as *mut _ as *mut std::os::raw::c_void,
)?;
Expand Down Expand Up @@ -276,19 +276,19 @@ impl Device {
}
control::Value::CompoundU8(ref val) => {
control.__bindgen_anon_1.p_u8 = val.as_ptr() as *mut u8;
control.size = (val.len() * std::mem::size_of::<u8>()) as u32;
control.size = std::mem::size_of_val(val.as_slice()) as u32;
}
control::Value::CompoundU16(ref val) => {
control.__bindgen_anon_1.p_u16 = val.as_ptr() as *mut u16;
control.size = (val.len() * std::mem::size_of::<u16>()) as u32;
control.size = std::mem::size_of_val(val.as_slice()) as u32;
}
control::Value::CompoundU32(ref val) => {
control.__bindgen_anon_1.p_u32 = val.as_ptr() as *mut u32;
control.size = (val.len() * std::mem::size_of::<u32>()) as u32;
control.size = std::mem::size_of_val(val.as_slice()) as u32;
}
control::Value::CompoundPtr(ref val) => {
control.__bindgen_anon_1.ptr = val.as_ptr() as *mut std::os::raw::c_void;
control.size = (val.len() * std::mem::size_of::<u8>()) as u32;
control.size = std::mem::size_of_val(val.as_slice()) as u32;
}
};

Expand All @@ -311,7 +311,7 @@ impl Device {
};

v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_S_EXT_CTRLS,
&mut controls as *mut _ as *mut std::os::raw::c_void,
)
Expand All @@ -323,7 +323,7 @@ impl io::Read for Device {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
unsafe {
let ret = libc::read(
self.handle().as_raw_fd(),
self.as_raw_fd(),
buf.as_mut_ptr() as *mut std::os::raw::c_void,
buf.len(),
);
Expand All @@ -339,7 +339,7 @@ impl io::Write for Device {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
unsafe {
let ret = libc::write(
self.handle().as_raw_fd(),
self.as_raw_fd(),
buf.as_ptr() as *const std::os::raw::c_void,
buf.len(),
);
Expand All @@ -358,17 +358,29 @@ impl io::Write for Device {
}
}

impl AsFd for Device {
fn as_fd(&self) -> BorrowedFd<'_> {
self.handle.as_fd()
}
}

impl AsRawFd for Device {
fn as_raw_fd(&self) -> RawFd {
self.handle.as_raw_fd()
}
}

/// Device handle for low-level access.
///
/// Acquiring a handle facilitates (possibly mutating) interactions with the device.
#[derive(Debug, Clone)]
pub struct Handle(RawFd);
#[derive(Debug)]
pub struct Handle(OwnedFd);

Comment on lines -364 to +377
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type always wrongly derived Clone, causing drop() close() the same fd multiple times...

Instead I'd argue this trait should be available on Device which holds on to an Arc<Handle> exactly to allow sharing of the fd without cloning it.

Additionally Handle could provide try_clone() to duplicate the file descriptor?


Also, Device currently has an fn handle() to clone this Arc<Handle> but thus far seems to mostly be used to distribute the Fd around. Hence:

  • Should we implement As(Raw)Fd for Device so that it's easier to borrow its (raw) file descriptor?
  • Should various implementations like io::Queue store a Device directly so that it's more obvious that it's not just a wrapper around OwnedFd but really a wrapper around a Device?

impl Handle {
/// Wraps an existing file descriptor
///
/// The caller must ensure that `fd` is a valid, open file descriptor for a V4L device.
pub unsafe fn new(fd: RawFd) -> Self {
pub fn new(fd: OwnedFd) -> Self {
Self(fd)
Comment on lines 380 to +383
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs, while still true, are a bit less relevant now (see also the removal of unsafe) because the caller already has to guarantee this when constructing an OwnedFd.

}

Expand All @@ -385,7 +397,7 @@ impl Handle {
return Err(io::Error::last_os_error());
}

Ok(Handle(fd))
Ok(Handle(unsafe { OwnedFd::from_raw_fd(fd) }))
}

/// Polls the file descriptor for I/O events
Expand All @@ -400,35 +412,38 @@ impl Handle {
pub fn poll(&self, events: i16, timeout: i32) -> io::Result<i32> {
match unsafe {
libc::poll(
[libc::pollfd {
fd: self.0,
&mut libc::pollfd {
fd: self.as_raw_fd(),
events,
revents: 0,
}]
.as_mut_ptr(),
},
1,
timeout,
)
} {
-1 => Err(io::Error::last_os_error()),
ret => {
// A return value of zero means that we timed out. A positive value signifies the
// number of fds with non-zero revents fields (aka I/O activity).
assert!(ret == 0 || ret == 1);
Ok(ret)
}
// A return value of zero means that we timed out. A positive value signifies the
// number of fds with non-zero revents fields (aka I/O activity).
ret @ 0..=1 => Ok(ret),
ret => panic!("Invalid return value {}", ret),
}
}
}

impl Drop for Handle {
fn drop(&mut self) {
let _ = v4l2::close(self.0);
impl AsFd for Handle {
fn as_fd(&self) -> BorrowedFd<'_> {
self.0.as_fd()
}
}

impl AsRawFd for Handle {
fn as_raw_fd(&self) -> RawFd {
self.0
self.0.as_raw_fd()
}
}

impl IntoRawFd for Handle {
fn into_raw_fd(self) -> RawFd {
self.0.into_raw_fd()
}
}
2 changes: 1 addition & 1 deletion src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
marker::PhantomData,
mem,
ops::{Index, IndexMut},
os::unix::io::AsRawFd,
os::fd::AsRawFd,
ptr, slice,
sync::Arc,
};
Expand Down Expand Up @@ -99,7 +99,7 @@
)?;
}

Ok(Metadata::from(buf))

Check failure on line 102 in src/io/mod.rs

View workflow job for this annotation

GitHub Actions / Check

mismatched types
}

/// Start the stream
Expand Down Expand Up @@ -200,7 +200,7 @@
///
/// * `buf` - Buffer metadata
pub fn enqueue(&mut self, buf: &Metadata) -> io::Result<()> {
let mut buf: v4l2_buffer = (*buf).into();

Check failure on line 203 in src/io/mod.rs

View workflow job for this annotation

GitHub Actions / Check

the trait bound `v4l_sys::v4l2_buffer: From<buffer::Metadata>` is not satisfied
buf.memory = Memory::Mmap as u32;

self.qbuf(&mut buf)
Expand All @@ -214,7 +214,7 @@
};

self.dqbuf(&mut buf)?;
Ok(Metadata::from(buf))

Check failure on line 217 in src/io/mod.rs

View workflow job for this annotation

GitHub Actions / Check

mismatched types
}
}

Expand Down Expand Up @@ -280,7 +280,7 @@
///
/// * `buf` - Buffer metadata
pub fn enqueue(&mut self, buf: &Metadata) -> io::Result<()> {
let mut buf: v4l2_buffer = (*buf).into();

Check failure on line 283 in src/io/mod.rs

View workflow job for this annotation

GitHub Actions / Check

the trait bound `v4l_sys::v4l2_buffer: From<buffer::Metadata>` is not satisfied
buf.memory = Memory::UserPtr as u32;
buf.m = v4l2_buffer__bindgen_ty_1 {
userptr: self.bufs[buf.index as usize].as_ptr() as std::os::raw::c_ulong,
Expand All @@ -297,7 +297,7 @@
};

self.dqbuf(&mut buf)?;
Ok(Metadata::from(buf))

Check failure on line 300 in src/io/mod.rs

View workflow job for this annotation

GitHub Actions / Check

mismatched types
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/video/capture/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl Capture for Device {
..mem::zeroed()
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_G_PARM,
&mut v4l2_params as *mut _ as *mut std::os::raw::c_void,
)?;
Expand All @@ -63,7 +63,7 @@ impl Capture for Device {
},
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_S_PARM,
&mut v4l2_params as *mut _ as *mut std::os::raw::c_void,
)?;
Expand Down
12 changes: 6 additions & 6 deletions src/video/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl traits::Video for Device {
loop {
let ret = unsafe {
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_ENUM_FRAMEINTERVALS,
&mut v4l2_struct as *mut _ as *mut std::os::raw::c_void,
)
Expand Down Expand Up @@ -65,7 +65,7 @@ impl traits::Video for Device {
loop {
let ret = unsafe {
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_ENUM_FRAMESIZES,
&mut v4l2_struct as *mut _ as *mut std::os::raw::c_void,
)
Expand Down Expand Up @@ -99,7 +99,7 @@ impl traits::Video for Device {

unsafe {
ret = v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_ENUM_FMT,
&mut v4l2_fmt as *mut _ as *mut std::os::raw::c_void,
);
Expand All @@ -121,7 +121,7 @@ impl traits::Video for Device {

unsafe {
ret = v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_ENUM_FMT,
&mut v4l2_fmt as *mut _ as *mut std::os::raw::c_void,
);
Expand All @@ -138,7 +138,7 @@ impl traits::Video for Device {
..mem::zeroed()
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_G_FMT,
&mut v4l2_fmt as *mut _ as *mut std::os::raw::c_void,
)?;
Expand All @@ -154,7 +154,7 @@ impl traits::Video for Device {
fmt: v4l2_format__bindgen_ty_1 { pix: (*fmt).into() },
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_S_FMT,
&mut v4l2_fmt as *mut _ as *mut std::os::raw::c_void,
)?;
Expand Down
4 changes: 2 additions & 2 deletions src/video/output/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl Output for Device {
..mem::zeroed()
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_G_PARM,
&mut v4l2_params as *mut _ as *mut std::os::raw::c_void,
)?;
Expand All @@ -63,7 +63,7 @@ impl Output for Device {
},
};
v4l2::ioctl(
self.handle().as_raw_fd(),
self.as_raw_fd(),
v4l2::vidioc::VIDIOC_S_PARM,
&mut v4l2_params as *mut _ as *mut std::os::raw::c_void,
)?;
Expand Down
Loading