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.

Sign up for GitHub

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

Jump to bottom

Store parsed hashes in DerivationOutput #3439

Merged
merged 18 commits into from
Jun 19, 2020
Merged

Store parsed hashes in DerivationOutput #3439

merged 18 commits into from
Jun 19, 2020

Conversation

Ericson2314
Copy link
Member

It's best to detect invalid data as soon as possible, with data types
that make storing it impossible.

@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch from 652f6a5 to ffcaf7a Compare March 24, 2020 05:04
@Ericson2314 Ericson2314 changed the title WIP: Store parsed hashes in DerivationOutput Store parsed hashes in DerivationOutput Mar 24, 2020
@Ericson2314
Copy link
Member Author

I had to add a bit more machinery to the C++ side Rust FFI to make this tick. Hopefully that makes it...no less safe than before :D.

@puckipedia
Copy link
Contributor

I think the Rust code will cause double frees, as writing into a pointer will also drop the value contained in it. You probably want to replace it with a ptr::write or something similar?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 24, 2020 •
edited
Loading

@puckipedia good catch! I wast thinking *mut on it's own was enough to avoid the double free, but I was wrong.

@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch 2 times, most recently from ae98bbb to 8ee0242 Compare March 24, 2020 14:34
@Ericson2314
Copy link
Member Author

Oh, I guess whatever compiler was being used on Darwin couldn't constexpras hard. Removing that which I only put in based on compilers errors, not actual need, anyways.

@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch 2 times, most recently from 97dad5c to a56a3cf Compare March 24, 2020 17:41
@Ericson2314 Ericson2314 changed the base branch from master to 0.5-release March 24, 2020 18:11
@Ericson2314 Ericson2314 changed the base branch from 0.5-release to master March 24, 2020 18:11
@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch from a56a3cf to 96cac09 Compare March 24, 2020 18:12
This was referenced Mar 25, 2020
Draft
Closed
@Ericson2314
Extend Rust FFI
e433d4a 
Do idiomatic C++ copy and move constructors for a few things, so
wrapping structs' defaults can work.
@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch 4 times, most recently from b674e10 to 3ad4f8f Compare March 26, 2020 14:42
@Ericson2314
Merge remote-tracking branch 'me/enum-FileIngestionMethod' into HEAD
f5494d9
@Ericson2314
Store parsed hashes in DerivationOutput
832bd53 
It's best to detect invalid data as soon as possible, with data types
that make storing it impossible.
@Ericson2314 Ericson2314 force-pushed the no-stringly-typed-derivation-output branch from 3ad4f8f to 832bd53 Compare March 30, 2020 15:35
@Ericson2314 Ericson2314 changed the title Store parsed hashes in DerivationOutput Store parsed hashes in DerivationOutput -- contains #3450 and #3455 Mar 30, 2020
@Ericson2314 Ericson2314 changed the title Store parsed hashes in DerivationOutput -- contains #3450 and #3455 Store parsed hashes in DerivationOutput -- contains #3450 May 28, 2020
@Ericson2314 Ericson2314 mentioned this pull request Jun 2, 2020
Merged
@Ericson2314 Ericson2314 changed the title Store parsed hashes in DerivationOutput -- contains #3450 Store parsed hashes in DerivationOutput Jun 17, 2020
@Ericson2314
Copy link
Member Author

OK! No more spooky FFI changes needed.

edolstra
edolstra reviewed Jun 19, 2020
View reviewed changes
src/libstore/store-api.hh
@@ -838,6 +839,9 @@ std::optional<ValidPathInfo> decodeValidPathInfo(
std::istream & str,
bool hashGiven = false);

/* Compute the prefix to the hash algorithm which indicates how the files were
ingested. */
std::string makeFileIngestionPrefix(const FileIngestionMethod m);
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, actually this is just a freestanding function and the closing brace is the namespace (so no indent). I guess in the later PR it gets a better home in content-address.h.

edolstra
edolstra reviewed Jun 19, 2020
View reviewed changes
src/libutil/hash.hh Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

OK! I think I got all of the comments, and CI has turned green.

@edolstra edolstra merged commit 424bb58 into NixOS:master Jun 19, 2020
@Ericson2314 Ericson2314 deleted the no-stringly-typed-derivation-output branch June 19, 2020 17:50
@ldesgoui ldesgoui mentioned this pull request Aug 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ericson2314 @puckipedia @edolstra @meditans