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

Improve EOF handling #36

Merged
merged 2 commits into from
Nov 5, 2020
Merged

Improve EOF handling #36

merged 2 commits into from
Nov 5, 2020

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 5, 2020

This came up from go-ipfs folks trying to import a CAR file that was truncated but they had no indication that it was. The final block had an 0xa0 after it, which is an improper varint, so ReadUvarint() was returning an io.EOF which also happens if there's no next block at all and is therefore the proper-EOF signal. This change lifts the proper-EOF case out into a Peek(1) so every other EOF from there on can be assumed to be unexpected.

@rvagg rvagg requested a review from mvdan November 5, 2020 06:16
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #36 into master will decrease coverage by 0.83%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   51.03%   50.20%   -0.84%     
==========================================
  Files           3        3              
  Lines         241      245       +4     
==========================================
  Hits          123      123              
- Misses         83       87       +4     
  Partials       35       35              
Impacted Files Coverage Δ
util/util.go 23.52% <0.00%> (-2.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6ccdc...cdd6313. Read the comment docs.

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great :) The CAR chunk of bytes would be a bit shorter if you used base64 by the way, via base64.StdEncoding.DecodeString.

@rvagg
Copy link
Member Author

rvagg commented Nov 5, 2020

thanks @mvdan, on the topic of bytes as strings: I prefer hex for for this kind of thing simply for its portability, this string for instance is straight out of JavaScript. Base64 tends to involve more dependencies and hoop-jumping in various languages, while hex is usually lowest-common-denominator

@rvagg rvagg merged commit c2f1ff2 into master Nov 5, 2020
@rvagg rvagg deleted the rvagg/eof branch November 5, 2020 23:53
masih added a commit that referenced this pull request Jul 15, 2021
Remove dependency to `bufio.Reader` in internal `carv1` package that
seems to be mainly used for peeking a byte to return appropriate error
when stream abruptly ends, relating to #36. This allows simplification
of code across the repo and remove all unnecessary wrappings of
`io.Reader` with `bufio.Reader`. This will also aid simplify the
internal IO utilities which will be done in future PRs. For now we
simply remove dependency to `bufio.Reader`

See:
- #36
mvdan pushed a commit that referenced this pull request Jul 16, 2021
Remove dependency to `bufio.Reader` in internal `carv1` package that
seems to be mainly used for peeking a byte to return appropriate error
when stream abruptly ends, relating to #36. This allows simplification
of code across the repo and remove all unnecessary wrappings of
`io.Reader` with `bufio.Reader`. This will also aid simplify the
internal IO utilities which will be done in future PRs. For now we
simply remove dependency to `bufio.Reader`

See:
- #36
Jorropo pushed a commit to ipfs/boxo that referenced this pull request Mar 22, 2023
Remove dependency to `bufio.Reader` in internal `carv1` package that
seems to be mainly used for peeking a byte to return appropriate error
when stream abruptly ends, relating to #36. This allows simplification
of code across the repo and remove all unnecessary wrappings of
`io.Reader` with `bufio.Reader`. This will also aid simplify the
internal IO utilities which will be done in future PRs. For now we
simply remove dependency to `bufio.Reader`

See:
- ipld/go-car#36


This commit was moved from ipld/go-car@cc1e449
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.

2 participants