Add NextReader to BlockReader #603
Conversation
Kubuxu
requested review from
hannahhoward,
masih,
rvagg and
willscott
as code owners
BlockReader.NextReader facilities reading larger blocks from the CAR file. Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
feat/next-reader
branch
from
97a3941 to
a1db313
Compare
v2/internal/carv1/util/util.go
Outdated
| if size > math.MaxInt64 { | ||
| return cid.Cid{}, 0, ErrHeaderTooLarge | ||
| } |
There was a problem hiding this comment.
| if size > math.MaxInt64 { | |
| return cid.Cid{}, 0, ErrHeaderTooLarge | |
| } |
this is unnecessary, it's what LdReadSize does with its third arg for you to generate a uniform error pattern
There was a problem hiding this comment.
also if this really is necessary, ErrSectionTooLarge cause this isn't actually a header.
There was a problem hiding this comment.
Added this because the int64 cast for LimitReader (don't want to cast to negatives)
v2/block_reader.go
Outdated
| // on the BlockReader. | ||
| // The returned length might be larger than MaxAllowedSectionSize, it is up to the user to check before loading the data into memory. | ||
| func (br *BlockReader) NextReader() (cid.Cid, io.Reader, uint64, error) { | ||
| c, length, err := util.ReadNodeHeader(br.r, br.opts.ZeroLengthSectionAsEOF, math.MaxUint64) |
There was a problem hiding this comment.
| c, length, err := util.ReadNodeHeader(br.r, br.opts.ZeroLengthSectionAsEOF, math.MaxUint64) | |
| c, length, err := util.ReadNodeHeader(br.r, br.opts.ZeroLengthSectionAsEOF, math.MaxInt64) |
Or is there a reason you're using MaxUint64 but then capping it at MaxInt64 inside ReadNodeHeader? Why not just use MaxInt64 all the way through.
There was a problem hiding this comment.
Because the maxsize param is uint64 everywhere, so from an external API viewpoint, I'm passing unlimited (maximum allowed value). Then the function internally caps to what it is able to handle.
Both perspectives make sense though
Signed-off-by: Jakub Sztandera <[email protected]>
|
@rvagg : will you be ok to merge and cut a release at your convenience as filecoin-project/lotus#13129 will be depending on it (being worked on next week). |
github-project-automation
bot
moved this from 🔎 Awaiting Review
to 🎉 Done
in nv27 Track Board
|
Just noting here for completeness for anyone looking on afterwards (probably should have noted in code) - in this PR we're bypassing some byte buffer size limitations we put in place to deal with security concerns a while a go, essentially saying "a section can be unlimited size, I don't even care what size the CID is" - but this is OK here because, (a) obviously we're doing an But, the important point here is that a CAR section head could claim that the section is a certain size and lie about it, forcing an implementation to allocate way too much memory, significantly larger than the CAR itself. |
BlockReader.NextReaderfacilities reading larger blocks from the CAR file.This is necessary for the support of the Filecoin snapshot format extension (FRC-108, lotus issue), where a large block is used to transfer data about F3.