google / coset

A set of Rust types for supporting COSE
Apache License 2.0
28 stars 16 forks source link

Remove MeasuringReader. #73

Closed AlanStokes closed 1 year ago

AlanStokes commented 1 year ago

Ciborium implements Read for &mut R for any R that itself implementats Read. So we can just pass our slice by reference rather than by value when decoding, and then we can directly check whether it has been fully consumed.

That simplifies things a bit. Unfortunately we then need to do some munging of the error type, which complicates things.

ciphergoth commented 1 year ago

Huh, neat, thanks for noticing this! Is there a test that validates that this works as expected ie from_reader mutates the slice to remove consumed data?

AlanStokes commented 1 year ago

Generally LGTM, I guess we just failed to notice that we could get extraneous data detection without the wrapper.

I came across it accidentally in a different context, using std::io::Read.

Is there a test that validates that this works as expected ie from_reader mutates the slice to remove consumed data?

test_label_decode_fail has a test for extraneous data which I think covers it?

ciphergoth commented 1 year ago

Yep, just double checked all that in the ciborium and std source, this all LGTM, but leaving final approval to David for the various nits. Wish I'd spotted this first time around, good catch!

ciphergoth commented 1 year ago

I think we want to get rid of our EndOfFile altogether. Or maybe this should be a typedef which is std::io::Error in std, and ciborium-io's EndOfFile in no-std.

daviddrysdale commented 1 year ago

a typedef which is std::io::Error in std, and ciborium-io's EndOfFile in no-std

I suspect it's a bad idea to have types that differ depending on activated features, given the way Cargo does feature unification.

ciphergoth commented 1 year ago

We sort of have that problem already, in that std varies what E is in cbor::de::from_reader(&mut [u8]) -> Result<T, cbor::de:error::Error<E>>. But if you think this is a bridge too far I respect that!

AlanStokes commented 1 year ago

I think we want to get rid of our EndOfFile altogether.

I think that's out of scope for this PR. And I don't have any strong opinion on it. It would technically be a breaking change, although I'd be surprised if anyone noticed.