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

memtest86-efi: compute ESP offset from partitions #86623

Merged
merged 1 commit into from May 4, 2020

Conversation

@emilazy
Copy link
Member

emilazy commented May 3, 2020

Motivation for this change

Uses sfdisk(1) + jq(1) rather than hardcoding a partition offset, and avoids an intermediate dd(1) step by passing the offset directly to mtools.

Hopefully the derivation is less obscure / better documented now.

cc @flokli @cdepillabout

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@emilazy emilazy requested a review from ajs124 May 3, 2020
@emilazy
Copy link
Member Author

emilazy commented May 3, 2020

Follow-up to #86470.

Copy link
Member

cdepillabout left a comment

@emilazy Thanks for sending a PR updating this.

I think this is looking really good. It is easy to understand what is going on here, and I feel comfortable keeping this updated in the future.


I have two small concerns, but I don't think they are big enough to prevent this from being merged in.

Like I said in #86470 (comment), it looks like we are still potentially passing both partitions to mtool, and relying on mtool to figure out it should disregard the second one. Although in this case, we could probably use a similar sfdisk call to figure out the length of the partition we want, and pull that out with dd, somewhat similar to what was done in #86470. I'm okay with not doing that here in this PR, and making that change in the future if it becomes a problem.

My second small concern is that it appears the output format of sfdisk changes slightly from version to version? I was playing around with sfdisk, and I was running into a weird problem of it not working. I realized I was using sfdisk from 19.09. The output format of the JSON is slightly different. It didn't contain the sectorsize key. This also isn't a big problem, and shouldn't prevent this from being merged in, but when I go to update this derivation, if it starts failing I'll have to keep this in mind as a possible cause.

@emilazy
Copy link
Member Author

emilazy commented May 3, 2020

Like I said in #86470 (comment), it looks like we are still potentially passing both partitions to mtool, and relying on mtool to figure out it should disregard the second one. Although in this case, we could probably use a similar sfdisk call to figure out the length of the partition we want, and pull that out with dd, somewhat similar to what was done in #86470. I'm okay with not doing that here in this PR, and making that change in the future if it becomes a problem.

Specifying the location of partitions is the intended use of the offset functionality (see e.g. the mtools manual), and in general is just how partitions and filesystems work. Filesystems generally encode their own extent, and there's no "dynamically-sized reads/writes" that would go outside their limits, so unless the image was maliciously constructed as invalid an FAT filesystem that points to sectors outside of the partition, there's no way mtools would even be aware of the rest of the image; that's presumably why it only takes an offset value to begin with, not an offset and a size. If the risk you were worrying about was real, then something like growing a partition would be much more risky and difficult, rather than the routine operation it is.

I don't claim that any of this is particularly obvious, but I think the fact that mtools accepts this syntax in the first place is suggestive of the fact that it's how things are meant to work, and I don't think a diversion about how partition tables and filesystem headers work would really be appropriate for this derivation.

My second small concern is that it appears the output format of sfdisk changes slightly from version to version? I was playing around with sfdisk, and I was running into a weird problem of it not working. I realized I was using sfdisk from 19.09. The output format of the JSON is slightly different. It didn't contain the sectorsize key. This also isn't a big problem, and shouldn't prevent this from being merged in, but when I go to update this derivation, if it starts failing I'll have to keep this in mind as a possible cause.

I guess the JSON API is probably pretty new; it was the least kludgy way I could find to parse GPT from userspace programmatically. If anyone knows a better alternative I'd be happy to use that instead.

@cdepillabout
Copy link
Member

cdepillabout commented May 3, 2020

Hmm, I've discovered something interesting.

If I go back to before #86470, the files output are slightly different:

$ cd /some/path/to/nixpkgs/
$ git checkout b7f80f00eff  # an older commit from before https://github.com/NixOS/nixpkgs/pull/86470 was merged in
$ nix-build -A memtest86-efi
$ ls result/
Benchmark  blacklist.cfg  BOOTIA32.efi  BOOTX64.efi  mt86.png  unifont.bin
$ git checkout 15172cf # the commit from this PR
$ nix-build -A memtest86-efi
$ ls result/
blacklist.cfg  BOOTIA32.efi  BOOTX64.efi  mt86.png  unifont.bin

It looks like the Benchmark/ directory is no longer being pulled out from the partition by mtool. This seems to be the case for both this PR (#86623) and the previous PR from @ajs124 (#86470).

After playing around with this for a while, I figured out that passing the -s flag to mcopy makes it successfully create the Benchmark/ directory.

I don't think this Benchmark/ directory is actually used for anything, so I don't think this is a problem, but it might be a good idea to pass the -s flag to mcopy in case there ever are subdirectories that contain useful content.

@cdepillabout
Copy link
Member

cdepillabout commented May 3, 2020

Like I said above, this LGTM, but I'll give @ajs124 a few days to do a review (since @ajs124 is the author of the original PR).

If @ajs124 doesn't respond in a few days, @emilazy please feel free to ping me and I will merge this in.

@emilazy emilazy force-pushed the emilazy:refactor-memtest86-efi branch from 15172cf to 3ae05b6 May 3, 2020
@ofborg ofborg bot requested a review from cdepillabout May 3, 2020
@cdepillabout cdepillabout mentioned this pull request May 3, 2020
3 of 10 tasks complete
@ajs124
ajs124 approved these changes May 3, 2020
@cdepillabout
Copy link
Member

cdepillabout commented May 4, 2020

LGTM!

@emilazy Thanks for sending this PR, and @ajs124 thanks for reviewing this.

@cdepillabout cdepillabout merged commit d9b517f into NixOS:master May 4, 2020
17 checks passed
17 checks passed
memtest86-efi, memtest86-efi.passthru.tests on aarch64-linux No attempt
Details
memtest86-efi, memtest86-efi.passthru.tests on x86_64-darwin No attempt
Details
memtest86-efi, memtest86-efi.passthru.tests on x86_64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="3ae05b6"; rev="3ae05b64b6b25ef6bdefa085ec485a9ed16afb44"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@emilazy emilazy deleted the emilazy:refactor-memtest86-efi branch May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.