[Cleanup] Audit: Replace panics (unwrap/expect) with Result in libwayshot

[0xsamalt]

Libwayshot currently has certain instances of unwrap() and I think replacing it to Result is good for the stability of library and they can cause host application to panic at runtime.

I am proposing to replace all the instances of unwrap() to Result with proper error msgs.

I have analyzed and found the following instances:

src/lib.rs:            bo.fd_for_plane(0).unwrap(),
src/dispatch.rs:                let format = state.formats.first_mut().unwrap();
src/dispatch.rs:                let set_format = state.formats.first_mut().unwrap();
src/dispatch.rs:                let device = u64::from_le_bytes(device.try_into().unwrap());
src/dispatch.rs:        Card(options.open(path).unwrap())

Proposed Implementation:
Convert affected functions to return Result<T, Error> with proper error messages indicating the failure reason.

[Decodetalkers]

This part is designed to know there is value. Maybe should use panic?

I do not know when bo.fd_for_plane(0) will be empty,

[0xsamalt]

That makes sense... I agree that bo.fd_for_plane(0) and the formats.first_mut() call in dispatch.rs are internal invariants and there failure would mean fundamental logical bug.

However, I think there is still significant value in converting the external-facing failures to Result:
options.open(path).unwrap(): it can fail due to permissions or device-busy states. A Result here prevents a full crash and allows the user to see a 'Permission Denied' error instead.
device.try_into().unwrap(): This conversion depends on data sent from the compositor. If a compositor sends malformed data, we should probably return a protocol error rather than panicking.

What do you think about them?

[Decodetalkers]

I agree, maybe the device part, we should use if let to keep the device will always not panic.

line 304 and line 323, it is ensured that it will always has a value, so I think it will be ok.

Card::open, that part you can help change it to save function, not use unwrap.

so, yes, you are right, at least these two part, we can avoid use unwrap

[Decodetalkers]

I think I would better use

&mut state.formats[0]

In line 304. That way will also make it unwrap less

[Anshi321]

Hi, I’d like to work on this. I’ll focus on replacing the agreed unwraps (options.open(path) and device.try_into()) with proper error handling as discussed above, and avoid modifying the internal invariants.

I’ll open a PR soon.

[0xsamalt]

Hi @Anshi321, I actually have the code for these specific changes already completed and undergoing final testing. Since I’ve been coordinating the technical implementation details with @Decodetalkers in this thread, I'll be submitting the PR shortly. Thanks for the interest, though!

[0xsamalt]

Following the discussion... I have opened a PR#294 with necessary changes. Looking forward to your feedback!
Link to the PR: #294

[Decodetalkers]

done