Consider making `Corrupt` and `Incompatible` opaque

[nicholasbishop]

Currently these types are public enums. I don't think this is ideal for creating a stable interface. The enums are marked non_exhaustive, so we can add variants without it being a breaking change, but we can't change the variants' data or remove variants.

And I don't think there's any good reason why a user would want to match on these values, so it's a needlessly expressive part of the API.

So I'm thinking of changing the API to something like:

pub struct Corrupt(CorruptKind);
pub struct Corrupt(IncompatibleKind);

pub(crate) enum CorruptKind {
   // All the variants that are currently in the Corrupt enum.
}

pub(crate) enum IncompatibleKind {
   // All the variants that are currently in the Incompatible enum.
}

The Debug/Display impls will still carry all the same information, you just won't be able to directly access it.

[nicholasbishop]

Initial PR for making Corrupt opaque is up. Making notes on a few more cleanups:

Add impl PartialEq<CorruptKind> for Ext4Error { ... }. This will allow changing tests like this:

assert!(matches!(Ext4::load(Box::new(vec![0; 2048])).unwrap_err(), Ext4Error::Corrupt(Corrupt(CorruptKind::SuperblockMagic))));

To this:

assert_eq!(Ext4::load(Box::new(vec![0; 2048])).unwrap_err(), CorruptKind::SuperblockMagic);

And that will allow making the Corrupt opaque even within the crate.

Also:

• Drop Ext4Error::as_corrupt.
• Drop the docstrings from CorruptKind. These are mostly duplicating the error strings anyway.

[avalon1610]

I have a scenario where I need to match variants of Corrupt. In my current code when parsing certain ext4 files, it requires ignoring some inode errors for the entire file to be fully parsed. e.g.

if let Ext4Error::Corrupt(Corrupt::Inode(i)) = e {
   debug!("corrupt inode {}, skip", i);
   continue;
}

The new 0.8 version has caused this part of my code to stop working.

[nicholasbishop]

Do you know if the filesystem is actually corrupt? I'm wondering if there's just a bug in the library, making some inodes appear corrupt, or if the corruption error is correct. Would you be able to share the filesystem with me in some way?

[avalon1610]

Sorry I can't share this filesystem because it's a company asset. It works fine when burned to an embedded device, and I don't know if it's really corrupted.

[nicholasbishop]

Understood. Perhaps we could find some way to get a better idea of the problem -- either adding logging to the crate, or making the inode corruption more detailed.

In the meantime, to address your issue: do you actually need to distinguish different types of corruption? Would it be sufficient in this case to do something like:

if let Ext4Error::Corrupt(e) = e {
   debug!("skipping corrupt file: {e}");
   continue;
}

[avalon1610]

I do need distinguish the different types (only ignore the inode corruption) for now. I need do more test to check if it is really needed.

[nicholasbishop]

When you get a chance, please test with the just-released 0.9.0. I have split the inode corruption error into several different errors, which will hopefully help us figure out more specifically what's failing. Then I can examine the code in that area and attempt to determine if there's a bug in the library.