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

Implement API to allow replacing root CIDs in a CARv1 or CARv2 #250

Merged
merged 1 commit into from Oct 1, 2021

Conversation

masih
Copy link
Member

@masih masih commented Sep 30, 2021

Implement an API that allows a caller to replace root CIDs in an
existing CAR file, may it be v1 or v2, as long as the resulting
serialized header is of identical size to the existing header.

Assert that the new API works in a variety of CARv1 and CARv2 files
along with failure scenarios.

Fixes #245

@masih masih marked this pull request as ready for review September 30, 2021 15:47
@mvdan
Copy link
Contributor

mvdan commented Sep 30, 2021

I think having it take a path on disk is fair - we only exposed OpenReadWrite after all, and no variant that attempts to use io interfaces, because we just need too many methods.

And if we want to later on expose a FinalizeWithRoots, we could also do that - and it would internally share the same implementation as ReplaceRootsInFile, perhaps via an unexported API that works on *os.File.

@masih
Copy link
Member Author

masih commented Sep 30, 2021

And if we want to later on expose a FinalizeWithRoots, we could also do that

That was my thought too. I think we probably need both: Basic APIs to work with CAR files, and equivalent functionality used through blockstore.

I think we have a separate ticket already for blockstore support: #196

@mvdan
Copy link
Contributor

mvdan commented Sep 30, 2021

Yep agreed. Was just thinking outloud :)

v2/writer_test.go Outdated Show resolved Hide resolved
v2/writer_test.go Outdated Show resolved Hide resolved
v2/writer_test.go Outdated Show resolved Hide resolved
v2/writer.go Outdated Show resolved Hide resolved
v2/writer.go Outdated Show resolved Hide resolved
v2/writer_test.go Outdated Show resolved Hide resolved
Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

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

we could potentially get rid of the comment saying it doesn't shift padding - i'd prefer that to show up in this method once supported, rather than maintaining two different exposed APIs for it.

Implement an API that allows a caller to replace root CIDs in an
existing CAR file, may it be v1 or v2, as long as the resulting
serialized header is of identical size to the existing header.

Assert that the new API works in a variety of CARv1 and CARv2 files
along with failure scenarios.

Fixes #245
@mvdan mvdan merged commit f437812 into master Oct 1, 2021
@masih masih deleted the masih/carv2-write-roots branch October 1, 2021 09:46
@mvdan
Copy link
Contributor

mvdan commented Oct 1, 2021

i'd prefer that to show up in this method once supported, rather than maintaining two different exposed APIs for it.

My only worry is that it would end up being an API that might be very cheap (just rewrite the first kilobyte of data) versus very expensive (copy potentially gigabytes of data, depending on how large the CAR is).

@masih
Copy link
Member Author

masih commented Oct 1, 2021

I think what Will is taking about is to use the padding after CARv2 header, before inner CARv1 to grow roots when needed. That should still be cheap for CARv2 files and it just means DataOffset in CARv2 header needs to be updated.

So we could expand the implementation to support replacing larger roots as long as they fit into the padding+current v1 header size

@mvdan
Copy link
Contributor

mvdan commented Oct 1, 2021

Ah, that's fair. As long as it's equally cheap :)

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.

Better ergonomics around updating car root.
3 participants