mctp-usb: Simply return the header in MctpUsbHandler::header()#46
Conversation
|
Looks good! The CI failure looks like a general thing from the nightly toolchain, which we may want to address separately. The change itself looks good, I would suggest a couple of things on the commit message though:
I'd suggest a prefix on this, as you have done in the PR. Since we're modifying multiple crates, perhaps I'd also suggest some more context in the commit message; for example, why would it be better, and what effects would this have on the public API? (are there users of |
I have a pending change here to remove nightly toolchain from the PR CI job, and run it separately on a timer. I'll push that soon as a separate PR. |
SafetyInObscurity
force-pushed
the
return-type
branch
2 times, most recently
from
d82c284 to
4110369
Compare
| match Self::header(len) { | ||
| Ok(header) => hdr.copy_from_slice(&header), | ||
| Err(_) => { | ||
| trace!("USB transfer error"); | ||
| return SendOutput::Error { | ||
| err: Error::TxFailure, | ||
| cookie: None, | ||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The error path can't be hit (wasn't possible before either, hence InternalError). The only failure from header() is when len > sizeof(u8), but MCTP_USB_MTU_MAX prevents that.
Could just have
let header = Self::header(len).unwrap();
hdr.copy_from_slice(&header);| @@ -150,22 +156,13 @@ impl MctpUsbHandler { | |||
| /// `mctplen` is the length of the remaining MCTP packet | |||
| /// after the header. | |||
| /// `hdr` must be a 4 byte slice. | |||
There was a problem hiding this comment.
No more hdr argument, can remove this line
SafetyInObscurity
force-pushed
the
return-type
branch
from
4110369 to
958f02f
Compare
Current implementation takes destination variable as a mutable argument and modifies it. It would be better for the function to simply return the header, rather than an empty `Ok(())` on success. This breaks the current API for header(), but adjustment should be simple. Signed-off-by: James Lee <james@codeconstruct.com.au>
SafetyInObscurity
force-pushed
the
return-type
branch
from
958f02f to
52e940e
Compare
3 participants
Current implementation takes destination variable as a mutable argument and modifies it. It would be better for the function to simply return the header, rather than an empty
Ok(())on success.