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.

Sign up for GitHub

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

Jump to bottom

sound: add clippy lint checks #514

Merged
merged 3 commits into from
Nov 2, 2023
Merged

sound: add clippy lint checks #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecf49b0
sound/pipewire: prevent unsigned sub overflow
epilys Oct 31, 2023
2a06bd9
sound: add package.categories field to Cargo.toml
epilys Oct 31, 2023
f7d5178
sound: add clippy lint checks
epilys Oct 31, 2023
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 staging/vhost-device-sound/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ description = "A virtio-sound device using the vhost-user protocol."
repository = "https://github.com/rust-vmm/vhost-device"
readme = "README.md"
keywords = ["vhost", "sound", "virtio-sound", "virtio-snd", "virtio"]
categories = ["multimedia::audio"]
epilys marked this conversation as resolved.
Show resolved Hide resolved
license = "Apache-2.0 OR BSD-3-Clause"
edition = "2018"

Expand Down
24 changes: 13 additions & 11 deletions staging/vhost-device-sound/src/audio_backends/alsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ fn update_pcm(

let period_frames = period_bytes / frame_size;

hwp.set_period_size(period_frames as i64, alsa::ValueOr::Less)?;
hwp.set_period_size(i64::from(period_frames), alsa::ValueOr::Less)?;

// Online ALSA driver recommendations seem to be that the buffer should be at
// least 2 * period_size.
Expand All @@ -177,7 +177,7 @@ fn update_pcm(
// > as well as the parameters set in the snd_pcm_hardware structure (in the driver).
//
// So, if the operation fails let's assume the ALSA runtime has set a better value.
if let Err(err) = hwp.set_buffer_size_near(2 * period_frames as i64) {
if let Err(err) = hwp.set_buffer_size_near(2 * i64::from(period_frames)) {
log::error!("could not set buffer size {}: {}", 2 * period_frames, err);
}

Expand Down Expand Up @@ -287,7 +287,7 @@ fn write_samples_io(
p: &alsa::PCM,
streams: &Arc<RwLock<Vec<Stream>>>,
stream_id: usize,
io: &mut alsa::pcm::IO<u8>,
io: &alsa::pcm::IO<u8>,
) -> AResult<bool> {
let avail = match p.avail_update() {
Ok(n) => n,
Expand Down Expand Up @@ -331,7 +331,7 @@ fn write_samples_io(
if buffer.pos as u32 >= buffer.desc_len() {
stream.buffers.pop_front();
}
p.bytes_to_frames(read_bytes as isize)
p.bytes_to_frames(isize::try_from(read_bytes).unwrap())
.try_into()
.unwrap_or_default()
})?;
Expand All @@ -352,7 +352,7 @@ fn read_samples_io(
p: &alsa::PCM,
streams: &Arc<RwLock<Vec<Stream>>>,
stream_id: usize,
io: &mut alsa::pcm::IO<u8>,
io: &alsa::pcm::IO<u8>,
) -> AResult<bool> {
let avail = match p.avail_update() {
Ok(n) => n,
Expand Down Expand Up @@ -394,7 +394,8 @@ fn read_samples_io(
.map(std::num::NonZeroUsize::get)
{
frames_read += frames;
let n_bytes = usize::try_from(p.frames_to_bytes(frames as i64)).unwrap_or_default();
let n_bytes =
usize::try_from(p.frames_to_bytes(frames.try_into().unwrap())).unwrap_or_default();
if buffer
.write_input(&intermediate_buf[0..n_bytes])
.expect("Could not write data to guest memory")
Expand All @@ -404,7 +405,7 @@ fn read_samples_io(
}
}

let bytes_read = p.frames_to_bytes(frames_read as i64);
let bytes_read = p.frames_to_bytes(frames_read.try_into().unwrap());
if buffer.pos as u32 >= buffer.desc_len() || bytes_read == 0 {
stream.buffers.pop_front();
}
Expand Down Expand Up @@ -456,9 +457,9 @@ fn alsa_worker(
continue 'empty_buffers;
}
} else {
let mut io = lck.io_bytes();
let io = lck.io_bytes();
// Direct mode unavailable, use alsa-lib's mmap emulation instead
if write_samples_io(&lck, &streams, stream_id, &mut io)? {
if write_samples_io(&lck, &streams, stream_id, &io)? {
continue 'empty_buffers;
}
}
Expand All @@ -475,8 +476,8 @@ fn alsa_worker(
continue 'empty_buffers;
}
} else {
let mut io = lck.io_bytes();
if read_samples_io(&lck, &streams, stream_id, &mut io)? {
let io = lck.io_bytes();
if read_samples_io(&lck, &streams, stream_id, &io)? {
continue 'empty_buffers;
}
}
Expand Down Expand Up @@ -505,6 +506,7 @@ impl AlsaBackend {
Self { sender, streams }
}

#[allow(clippy::cognitive_complexity)]
fn run(
streams: Arc<RwLock<Vec<Stream>>>,
receiver: Receiver<AlsaAction>,
Expand Down
4 changes: 2 additions & 2 deletions staging/vhost-device-sound/src/audio_backends/null.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ mod tests {
let streams = Arc::new(RwLock::new(vec![Stream::default()]));
let null_backend = NullBackend::new(streams.clone());

assert!(null_backend.write(0).is_ok());
null_backend.write(0).unwrap();

let streams = streams.read().unwrap();
assert_eq!(streams[0].buffers.len(), 0);
Expand All @@ -48,7 +48,7 @@ mod tests {
let streams = Arc::new(RwLock::new(vec![Stream::default()]));
let null_backend = NullBackend::new(streams.clone());

assert!(null_backend.read(0).is_ok());
null_backend.read(0).unwrap();

// buffer lengths should remain unchanged
let streams = streams.read().unwrap();
Expand Down
42 changes: 24 additions & 18 deletions staging/vhost-device-sound/src/audio_backends/pipewire.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::{
collections::HashMap,
convert::TryInto,
convert::TryFrom,
mem::size_of,
ptr,
sync::{Arc, RwLock},
Expand Down Expand Up @@ -70,6 +70,8 @@ unsafe impl Send for PwBackend {}
// is protected with a lock.
unsafe impl Sync for PwBackend {}

// FIXME: make PwBackend impl Send on all fields.
#[allow(clippy::non_send_fields_in_send_ty)]
pub struct PwBackend {
pub stream_params: Arc<RwLock<Vec<Stream>>>,
thread_loop: ThreadLoop,
Expand Down Expand Up @@ -274,7 +276,7 @@ impl AudioBackend for PwBackend {
_ => 44100,
},
flags: 0,
channels: params.channels as u32,
channels: u32::from(params.channels),
position: pos,
};

Expand Down Expand Up @@ -362,8 +364,10 @@ impl AudioBackend for PwBackend {
};

let mut buf_pos = buffer.pos;
let avail = (buffer.desc_len() as usize - buf_pos) as i32;
let n_bytes = n_samples.min(avail.try_into().unwrap());
let avail = usize::try_from(buffer.desc_len())
.unwrap()
.saturating_sub(buf_pos);
let n_bytes = n_samples.min(avail);
let p = &slice[start..start + n_bytes];

if buffer
Expand Down Expand Up @@ -400,13 +404,15 @@ impl AudioBackend for PwBackend {

let mut start = buffer.pos;

let avail = (buffer.desc_len() - start as u32) as i32;
let avail = usize::try_from(buffer.desc_len())
.unwrap()
.saturating_sub(start);

if avail < n_bytes as i32 {
n_bytes = avail.try_into().unwrap();
if avail < n_bytes {
n_bytes = avail;
}
let p = &mut slice[0..n_bytes];
if avail <= 0 {
if avail == 0 {
// SAFETY: We have assured above that the pointer is not
// null
// safe to zero-initialize the pointer.
Expand Down Expand Up @@ -435,8 +441,8 @@ impl AudioBackend for PwBackend {
};
let chunk = data.chunk_mut();
*chunk.offset_mut() = 0;
*chunk.stride_mut() = frame_size as _;
*chunk.size_mut() = n_bytes as _;
*chunk.stride_mut() = i32::try_from(frame_size).unwrap();
*chunk.size_mut() = u32::try_from(n_bytes).unwrap();
}
};
}
Expand Down Expand Up @@ -582,7 +588,7 @@ mod tests {

mem.write_obj::<R>(hdr, desc_out.addr()).unwrap();
vq.desc_table().store(index, desc_out).unwrap();
next_addr += desc_out.len() as u64;
next_addr += u64::from(desc_out.len());
index += 1;

// In response descriptor
Expand Down Expand Up @@ -635,19 +641,19 @@ mod tests {
let pw_backend = PwBackend::new(stream_params);
assert_eq!(pw_backend.stream_hash.read().unwrap().len(), 0);
assert_eq!(pw_backend.stream_listener.read().unwrap().len(), 0);
assert!(pw_backend.prepare(0).is_ok());
assert!(pw_backend.start(0).is_ok());
assert!(pw_backend.stop(0).is_ok());
pw_backend.prepare(0).unwrap();
pw_backend.start(0).unwrap();
pw_backend.stop(0).unwrap();
let msg = ctrlmsg();
assert!(pw_backend.set_parameters(0, msg).is_ok());
pw_backend.set_parameters(0, msg).unwrap();
let release_msg = ctrlmsg();
assert!(pw_backend.release(0, release_msg).is_ok());
assert!(pw_backend.write(0).is_ok());
pw_backend.release(0, release_msg).unwrap();
pw_backend.write(0).unwrap();

let streams = streams.read().unwrap();
assert_eq!(streams[0].buffers.len(), 0);

assert!(pw_backend.read(0).is_ok());
pw_backend.read(0).unwrap();
}

#[test]
Expand Down
56 changes: 25 additions & 31 deletions staging/vhost-device-sound/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ impl VhostUserSoundThread {
Ok(false)
}

#[allow(clippy::cognitive_complexity)]
fn process_control(
&self,
vring: &VringRwLock,
Expand Down Expand Up @@ -209,6 +210,7 @@ impl VhostUserSoundThread {
for i in chmaps.iter().skip(start_id).take(count) {
buf.extend_from_slice(i.as_slice());
}
drop(chmaps);
desc_chain
.memory()
.write_slice(&buf, desc_response.addr())
Expand Down Expand Up @@ -244,6 +246,7 @@ impl VhostUserSoundThread {
for i in jacks.iter().skip(start_id).take(count) {
buf.extend_from_slice(i.as_slice());
}
drop(jacks);
desc_chain
.memory()
.write_slice(&buf, desc_response.addr())
Expand Down Expand Up @@ -297,6 +300,7 @@ impl VhostUserSoundThread {
p.channels_max = s.channels_max;
buf.extend_from_slice(p.as_slice());
}
drop(streams);
desc_chain
.memory()
.write_slice(&buf, desc_response.addr())
Expand Down Expand Up @@ -519,10 +523,9 @@ impl VhostUserSoundThread {
IoState::WaitingBufferForStreamId(stream_id)
if descriptor.len() as usize == size_of::<VirtioSoundPcmStatus>() =>
{
let mut streams = self.streams.write().unwrap();
for b in std::mem::take(&mut buffers) {
streams[stream_id as usize].buffers.push_back(b);
}
self.streams.write().unwrap()[stream_id as usize]
.buffers
.extend(std::mem::take(&mut buffers).into_iter());
state = IoState::Done;
}
IoState::Ready
Expand Down Expand Up @@ -574,7 +577,7 @@ impl VhostUserSoundThread {
}

if !stream_ids.is_empty() {
let b = audio_backend.write().unwrap();
let b = audio_backend.read().unwrap();
match direction {
Direction::Output => {
for id in stream_ids {
Expand Down Expand Up @@ -653,17 +656,17 @@ impl VhostUserSoundBackend {
streams_no,
)?),
RwLock::new(VhostUserSoundThread::new(
chmaps.clone(),
jacks.clone(),
chmaps,
jacks,
vec![RX_QUEUE_IDX],
streams.clone(),
streams_no,
)?),
]
} else {
vec![RwLock::new(VhostUserSoundThread::new(
chmaps.clone(),
jacks.clone(),
chmaps,
jacks,
vec![
CONTROL_QUEUE_IDX,
EVENT_QUEUE_IDX,
Expand Down Expand Up @@ -858,7 +861,6 @@ mod tests {
let thread =
VhostUserSoundThread::new(chmaps, jacks, queue_indexes, streams.clone(), streams_no);

assert!(thread.is_ok());
let mut t = thread.unwrap();

// Mock memory
Expand All @@ -874,21 +876,18 @@ mod tests {
];

let audio_backend =
RwLock::new(alloc_audio_backend(config.audio_backend, streams.clone()).unwrap());
assert!(t
.handle_event(CONTROL_QUEUE_IDX, &vrings, &audio_backend)
.is_ok());
RwLock::new(alloc_audio_backend(config.audio_backend, streams).unwrap());
t.handle_event(CONTROL_QUEUE_IDX, &vrings, &audio_backend)
.unwrap();

let vring = VringRwLock::new(mem, 0x1000).unwrap();
vring.set_queue_info(0x100, 0x200, 0x300).unwrap();
vring.set_queue_ready(true);
assert!(t.process_control(&vring, &audio_backend).is_ok());
assert!(t
.process_io(&vring, &audio_backend, Direction::Output)
.is_ok());
assert!(t
.process_io(&vring, &audio_backend, Direction::Input)
.is_ok());
t.process_control(&vring, &audio_backend).unwrap();
t.process_io(&vring, &audio_backend, Direction::Output)
.unwrap();
t.process_io(&vring, &audio_backend, Direction::Input)
.unwrap();
}

#[test]
Expand All @@ -909,15 +908,14 @@ mod tests {
);

let audio_backend =
RwLock::new(alloc_audio_backend(config.audio_backend, streams.clone()).unwrap());
RwLock::new(alloc_audio_backend(config.audio_backend, streams).unwrap());

let vring = VringRwLock::new(mem, 0x1000).unwrap();
vring.set_queue_info(0x100, 0x200, 0x300).unwrap();
vring.set_queue_ready(true);
assert!(t.process_control(&vring, &audio_backend).is_err());
assert!(t
.process_io(&vring, &audio_backend, Direction::Output)
.is_err());
t.process_control(&vring, &audio_backend).unwrap_err();
t.process_io(&vring, &audio_backend, Direction::Output)
.unwrap_err();
}

#[test]
Expand Down Expand Up @@ -962,7 +960,7 @@ mod tests {
.unwrap();
vrings[RX_QUEUE_IDX as usize].set_queue_ready(true);

assert!(backend.update_memory(mem).is_ok());
backend.update_memory(mem).unwrap();

let queues_per_thread = backend.queues_per_thread();
assert_eq!(queues_per_thread.len(), 1);
Expand All @@ -976,19 +974,15 @@ mod tests {
exit.unwrap().write(1).unwrap();

let ret = backend.handle_event(CONTROL_QUEUE_IDX, EventSet::IN, &vrings, 0);
assert!(ret.is_ok());
assert!(!ret.unwrap());

let ret = backend.handle_event(EVENT_QUEUE_IDX, EventSet::IN, &vrings, 0);
assert!(ret.is_ok());
assert!(!ret.unwrap());

let ret = backend.handle_event(TX_QUEUE_IDX, EventSet::IN, &vrings, 0);
assert!(ret.is_ok());
assert!(!ret.unwrap());

let ret = backend.handle_event(RX_QUEUE_IDX, EventSet::IN, &vrings, 0);
assert!(ret.is_ok());
assert!(!ret.unwrap());

test_dir.close().unwrap();
Expand Down
Loading