uki: Get raw section from UKI #198
Conversation
crates/composefs-boot/src/uki.rs
Outdated
| image: &'a [u8], | ||
| section_name: &'static str, | ||
| ) -> Option<Result<&'a str, UkiError>> { | ||
| get_section(image, section_name).map(|res| { |
There was a problem hiding this comment.
can't you just use ?.and_then for that instead of the map? None on an Option works the same way as Err...
There was a problem hiding this comment.
We need map to get an Option. Doing just ?.and_then converts the entire thing to just a Result. But yeah, if we simplify the return type to just Result<..> then we could get away without the map
There was a problem hiding this comment.
I think in any case this function ought not to return Option<Result<..>>... the None return from the inner function is meant to mean "PortableExecutableError" so we should just convert that in the "text" variant of the function, I think... Having a separate None possibility makes no sense here...
There was a problem hiding this comment.
Agreed. Updated the PR
| image: &'a [u8], | ||
| section_name: &'static str, | ||
| ) -> Option<Result<&'a str, UkiError>> { | ||
| ) -> Option<Result<&'a [u8], UkiError>> { |
There was a problem hiding this comment.
Is there anything left in this function that returns Some(Err(..))? I think you could simplify the return value to just be Option<&'a [u8]> now, no?
There was a problem hiding this comment.
We do have
if pe_header.pe_magic != PE_MAGIC {
return None;
}which maybe could be an error instead?
There was a problem hiding this comment.
The final return value has to be a Result as all of the zerocopy functions return Results. We can probably do with a Result return type
There was a problem hiding this comment.
There's a comment before the function explaining how we convert all of the Result to Option because the error return type of the zerocopy functions is evil. That's what all of that .ok()? all over the place is about.
There was a problem hiding this comment.
I see. But still, returning just an Option feels weird as we lose info like UkiError::MissingSection / UTF-8 string conversion error, etc
There was a problem hiding this comment.
ah indeed the part where we return MissingSection is just out of the context of the diff so I missed it :)
allisonkarlitskaya
left a comment
allisonkarlitskaya
left a comment
There was a problem hiding this comment.
This looks more or less correct now, minus one question. Can you squash the commits since the second one is just deleting most of the code from the first one?
| let (pe_header, rest) = PeHeader::ref_from_prefix(rest).ok()?; | ||
| if pe_header.pe_magic != PE_MAGIC { | ||
| return None; | ||
| return Some(Err(UkiError::PortableExecutableError)); |
There was a problem hiding this comment.
Was this change required?
There was a problem hiding this comment.
Not really, but thought it conveys the intent better? If there is a header mismatch then we can consider it as an error
Add a function to get the raw section out of a UKI without converting it to a string. Useful in bootc for checking kernel skew while performing soft-reboot Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com> uki: Refactor `get_text_section` to return `Result` In `get_section` return an error on PE Header mismatch Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
uki-raw-section
branch
from
5c42762 to
7c44803
Compare
|
Squashed the commits into one |
2 participants
Add a function to get the raw section out of a UKI without converting it to a string.
Useful in bootc for checking kernel skew while performing soft-reboot