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

Reader shouldn't require its inner type to be a mutable reference #521

Merged

Conversation

wgreenberg
Copy link
Contributor

This makes Reader more general, so it can e.g. take ownership of a Read + Seek type.

@wcampbell0x2a
Copy link
Collaborator

Seem plausible, I needed to do the following in backhand for this patch:

diff --git a/backhand/src/metadata.rs b/backhand/src/metadata.rs
index 89b4994..bf76adf 100644
--- a/backhand/src/metadata.rs
+++ b/backhand/src/metadata.rs
@@ -117,12 +117,13 @@ pub fn read_block<R: Read + Seek>(
     superblock: &SuperBlock,
     kind: &Kind,
 ) -> Result<Vec<u8>, BackhandError> {
-    let mut deku_reader = Reader::new(reader);
+    let mut deku_reader = Reader::new(&mut *reader);
     let metadata_len = u16::from_reader_with_ctx(&mut deku_reader, kind.inner.data_endian)?;

     let byte_len = len(metadata_len);
     tracing::trace!("len: 0x{:02x?}", byte_len);
     let mut buf = vec![0u8; byte_len as usize];
+    // deku_reader.as_mut().read_exact(&mut buf)?; // or this instead of the above
     reader.read_exact(&mut buf)?;

     let bytes = if is_compressed(metadata_len) {
diff --git a/backhand/src/squashfs.rs b/backhand/src/squashfs.rs
index b4420c5..2240d3c 100644
--- a/backhand/src/squashfs.rs
+++ b/backhand/src/squashfs.rs
@@ -225,7 +225,7 @@ impl<'b> Squashfs<'b> {
         kind: &Kind,
     ) -> Result<(SuperBlock, Option<CompressionOptions>), BackhandError> {
         // Parse SuperBlock
-        let mut container = Reader::new(reader);
+        let mut container = Reader::new(&mut *reader);
         let superblock = SuperBlock::from_reader_with_ctx(
             &mut container,
             (

@wgreenberg
Copy link
Contributor Author

ack, it turns out this is something of a compiler limitation: rust won't automatically reborrow generics, hence why you have to manually reborrow. one way to rewrite backhand's usage would be:

            let mut my_reader = Reader::new(reader);
            _ = u16::from_reader_with_ctx(&mut my_reader, ()).unwrap();

            let reader = my_reader.into_inner();
            reader.read_exact(&mut vec![0; 10]).unwrap();

but i understand if you think this is too obtuse

@wcampbell0x2a
Copy link
Collaborator

ack, it turns out this is something of a compiler limitation: rust won't automatically reborrow generics, hence why you have to manually reborrow. one way to rewrite backhand's usage would be:

            let mut my_reader = Reader::new(reader);
            _ = u16::from_reader_with_ctx(&mut my_reader, ()).unwrap();

            let reader = my_reader.into_inner();
            reader.read_exact(&mut vec![0; 10]).unwrap();

but i understand if you think this is too obtuse

I'm not against the change, it's technically less restrictive. Do you have a source for the "rust won't automatically reborrow generics"?, I've run into this before and never went any further into research of the problem.

@sharksforarms
Copy link
Owner

I'm in favor of being more general. Satisfyingly, the tests also pass. Does this mean it's backwards compat? Any more tests that should be added? wdyt @wcampbell0x2a @wgreenberg ?

@wcampbell0x2a
Copy link
Collaborator

I'm in favor of being more general. Satisfyingly, the tests also pass. Does this mean it's backwards compat? Any more tests that should be added? wdyt @wcampbell0x2a @wgreenberg ?

I'm fine with this change. It's backwards compat for our tests, but it defiantly changes our public API, as above I had to change some of backhand.

@wcampbell0x2a wcampbell0x2a merged commit da0ac84 into sharksforarms:master Mar 19, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants