Read CAR Files #254
Read CAR Files #254
Conversation
ec2
requested review from
ansermino,
austinabell and
dutterbutter
as code owners
ipld/car/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Loads a CAR buffer into a BlockStore | ||
| pub fn load_car<R: Read, B: BlockStore>(s: &mut B, buf_reader: BufReader<R>) -> Result<(), Error> { |
There was a problem hiding this comment.
You don't need a mutable reference to the database, this will cause a headache for anyone using
There was a problem hiding this comment.
True. I just assumed you need it as mut becuase we're writing, but that is in fact not the case. fixed
ipld/car/src/lib.rs
Outdated
| let block = car_reader.next_block()?; | ||
| let check_cid = Cid::new_from_prefix(&block.cid.prefix(), &block.data) | ||
| .map_err(|e| Error::Other(e.to_string()))?; | ||
| assert_eq!(&check_cid, &block.cid); |
There was a problem hiding this comment.
remove the assert now, you shouldn't be asserting on inputs not handled in this utility
ipld/car/src/lib.rs
Outdated
| let (_len, buf) = ld_read(&mut buf_reader)?; | ||
| let header: CarHeader = from_slice(&buf).map_err(|e| Error::ParsingError(e.to_string()))?; | ||
| if header.roots.is_empty() { | ||
| return Err(Error::ParsingError("empty CAR file".to_owned())); | ||
| } | ||
| if header.version != 1 { | ||
| return Err(Error::InvalidFile("CAR file version must be 1".to_owned())); | ||
| } |
There was a problem hiding this comment.
arguably this header read should not be in the reader constructor? Thoughts?
There was a problem hiding this comment.
Should be in the reader constructor, I'd say. This allows us to parse and verify the header to fail early if its not a valid header
ipld/car/src/util.rs
Outdated
| } | ||
|
|
||
| pub(crate) fn read_node<R: Read>(buf_reader: &mut R) -> Result<(Cid, Vec<u8>), Error> { | ||
| let (_len, buf) = ld_read(buf_reader)?; |
There was a problem hiding this comment.
why is this first read discarded?
There was a problem hiding this comment.
actually ld_read doesnt need to return len. So i will remove
ipld/car/src/util.rs
Outdated
| pub(crate) fn read_cid(buf: &[u8]) -> Result<(Cid, u64), Error> { | ||
| // TODO: Add some checks for cid v0 | ||
| let (version, buf) = | ||
| unsigned_varint::decode::u64(buf).map_err(|e| Error::ParsingError(e.to_string()))?; | ||
| let (codec, multihash_with_data) = | ||
| unsigned_varint::decode::u64(buf).map_err(|e| Error::ParsingError(e.to_string()))?; | ||
| // multihash part | ||
| let (_hashcode, buf) = unsigned_varint::decode::u64(multihash_with_data) | ||
| .map_err(|e| Error::ParsingError(e.to_string()))?; | ||
| let hashcode_len_diff = multihash_with_data.len() - buf.len(); | ||
| let (len, _) = | ||
| unsigned_varint::decode::u64(buf).map_err(|e| Error::ParsingError(e.to_string()))?; | ||
|
|
||
| let cid: Cid = Cid::new( | ||
| cid::Codec::from(codec)?, | ||
| cid::Version::from(version)?, | ||
| cid::multihash::Multihash::from_bytes( | ||
| multihash_with_data[0..=(len as usize + hashcode_len_diff)].to_vec(), | ||
| )?, | ||
| ); | ||
| let len = cid.to_bytes().len() as u64; | ||
| Ok((cid, len)) |
There was a problem hiding this comment.
It doesn't make sense to me why you convert the reader into a slice, then to use it here as a reader. This seems like logic that should be attached to the Cid crate. Thoughts on making this take in a reader and attaching it to Cid crate? You wouldn't need that ld_read function or all this indexing that could panic the program on a bad input
There was a problem hiding this comment.
Added a todo for read_Cid using a bufreader
ipld/car/src/error.rs
Outdated
| use self::Error::*; | ||
|
|
||
| match self { | ||
| ParsingError(_) => "Failed to parse CAR file", | ||
| Error::InvalidFile(_) => "Invalid CAR file", | ||
| Other(_) => "Other Cid Error", |
There was a problem hiding this comment.
why the use if you are going to specify the enum prefix? Also, this is consuming the error messages, what is the point of this error type? If it doesn't matter then just remove this type and make the error type &'static str
ipld/car/src/lib.rs
Outdated
| use blockstore::BlockStore; | ||
| use cid::Cid; | ||
| use forest_encoding::from_slice; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::io::{BufReader, Read}; | ||
|
|
||
| mod error; | ||
| mod util; | ||
| use crate::util::read_node; | ||
| use error::*; | ||
| use util::ld_read; |
There was a problem hiding this comment.
nit: can you move the mods to the top and group all uses together?
Co-Authored-By: Austin Abell <austinabell8@gmail.com>
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #222
Other information and links