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: replace p7zip with mtools #86470

Merged
merged 1 commit into from May 1, 2020

Conversation

@ajs124
Copy link
Member

ajs124 commented May 1, 2020

Motivation for this change

Fix build after #86417

cc @flokli

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.
and dd
@emilazy
emilazy approved these changes May 1, 2020
Copy link
Member

emilazy left a comment

Looks good, thank you!

@GrahamcOfBorg build memtest86-efi

@flokli
Copy link
Contributor

flokli commented May 1, 2020

md5sum'ed everything before the p7zip deprecation and after this PR - we still get the same binaries, so LGTM. Thanks a lot!

@flokli flokli merged commit 448db2e into NixOS:master May 1, 2020
16 checks passed
16 checks passed
memtest86-efi, memtest86-efi.passthru.tests on aarch64-linux 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="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./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="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ac8066c"; rev="ac8066c144c76dfbc99a866049b61acaaccb470f"; } ./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
@flokli
Copy link
Contributor

flokli commented May 1, 2020

backported to 20.03 in f466a34.

@ajs124 ajs124 deleted the helsinki-systems:memtest86-efi branch May 1, 2020
7z x -o$TEMP/temp-efi-dirs $src/memtest86-usb.img
7z x -o$TEMP/memtest86-files $TEMP/temp-efi-dirs/EFI\ System\ Partition.img
cp -r $TEMP/memtest86-files/EFI/BOOT/* $out/
dd if=$src/memtest86-usb.img of=$TEMP/ESP.img skip=2048

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 2, 2020

Member

@ajs124 Thanks for sending this PR.

Could you send a follow-up PR adding a comment about how this dd and mcopy combination works? Why are you skipping the first 2048 bytes with the dd command?

This comment has been minimized.

Copy link
@emilazy

emilazy May 2, 2020

Member

It's actually the first 2048 (512-byte) sectors, as is the default of both dd and most legacy disks:

emily@renko ~/t/m/memtest86-usb.img> sfdisk -l memtest86-usb.img 
Disk memtest86-usb.img: 500 MiB, 524288000 bytes, 1024000 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: 68264C0F-858A-49F0-B692-195B64BE4DD7

Device              Start     End Sectors  Size Type
memtest86-usb.img1   2048  512000  509953  249M Microsoft basic data
memtest86-usb.img2 514048 1023966  509919  249M EFI System

So this is skipping the partition table; that means it's actually looking at the basic data partition, not the ESP, but it seems they both contain identical copies of all the files we need. Here's an overengineered alternative:

diff --git a/pkgs/tools/misc/memtest86-efi/default.nix b/pkgs/tools/misc/memtest86-efi/default.nix
index f2adadc0840..3a39ab88ce4 100644
--- a/pkgs/tools/misc/memtest86-efi/default.nix
+++ b/pkgs/tools/misc/memtest86-efi/default.nix
@@ -1,4 +1,4 @@
-{ fetchzip, lib, stdenv, mtools }:
+{ fetchzip, lib, stdenv, mtools, utillinux, jq }:
 
 stdenv.mkDerivation rec {
   pname = "memtest86-efi";
@@ -22,7 +22,7 @@ stdenv.mkDerivation rec {
     stripRoot = false;
   };
 
-  nativeBuildInputs = [ mtools ];
+  nativeBuildInputs = [ mtools utillinux jq ];
 
   installPhase = ''
     mkdir -p $out $TEMP/memtest86-files
@@ -32,7 +32,12 @@ stdenv.mkDerivation rec {
     #
     # The following uses dd and mcopy to extract the actual EFI app from the
     # usb image so that it can be installed directly on the hard drive.
-    dd if=$src/memtest86-usb.img of=$TEMP/ESP.img skip=2048
+    dd if=$src/memtest86-usb.img of=$TEMP/ESP.img $(
+      sfdisk --json memtest86-usb.img | jq -r '.partitiontable |
+        "bs=\(.sectorsize) " +
+        # Select the EFI system partition
+        (.partitions[] | select(.type == "C12A7328-F81F-11D2-BA4B-00A0C93EC93B")
+          | "skip=\(.start) count=\(.size)")')
     mcopy -i $TEMP/ESP.img ::/EFI/BOOT/ $TEMP/memtest86-files/
     mv $TEMP/memtest86-files/BOOT/* $out/
   '';

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 2, 2020

Member

@emilazy Thanks for describing this.

So this is skipping the partition table; that means it's actually looking at the basic data partition, not the ESP, but it seems they both contain identical copies of all the files we need.

Is this an okay assumption to make?

@emilazy @flokli @ajs124 Without any comments/documentation on this, I'll probably just end up reverting this PR next time I need to update memtest86-efi, unless it "just works".

This comment has been minimized.

Copy link
@emilazy

emilazy May 2, 2020

Member

If it's not a safe assumption to make, then it's easy enough to just select the EFI system partition instead, like my diff does. If people would prefer the additional dependencies of the explicit partition table parsing then I can open a PR for it; I just figured it might be a little excessive.

Please don't just wholesale revert a PR that removed a dependency on an unmaintained, insecure package, though ^^;

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 2, 2020

Member

@emilazy From my perspective, this derivation that I've been maintaining was updated to use a combination of dd and mtool, but no documentation was added explaining how this works (or why it is safe skipping the first 2048 sectors, or why it is safe to include both partitions in the thing we pass to mtool, or the C12A7328-F81F-11D2-BA4B-00A0C93EC93B string in your diff).

If this doesn't "just work" when I go to update it, and there is no documentation explaining why it works this way, then the easiest thing for me to do is just revert it.

Also, I haven't followed the removal of p7zip, but it seems like somewhat of a stretch to call out its insecurity here? The image memtest86 image here is considered trusted. It should be safe to pass to p7zip.

This comment has been minimized.

Copy link
@emilazy

emilazy May 2, 2020

Member

Unfortunately, your continued insistence that you will unilaterally revert others' changes unless other people do work ahead of time to your precise satisfaction makes me reluctant to invest further effort into contributing to this package, especially as I don't personally use it. I hope my patch and explanations can be useful in making the derivation more future-proof and well-documented, and hereby release them under the CC0 1.0 Universal Public Domain Dedication for the benefit of anyone who wants to use them, but regret that I was unable to convince you to adopt a more collaborative approach to maintenance.

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 2, 2020

Member

@emilazy I'm sorry to have put you off in this way. I'm guessing it was mostly from the comments about possibly reverting the changes in this PR. I wanted to make it clear that it was a possibility if I had trouble when updating memtest86-efi in the future, but I didn't intend to threaten you (or @ajs124). I appologize for this.

I do appreciate your explanations above, especially #86470 (comment).

I hope you can understand that having good comments in the code is a very important part of the maintainer-ship in nixpkgs. It makes it possible for people to help with things even if they are not super familiar with the derivation in question.


Also, maybe I didn't make this completely clear above, but I would definitely prefer not to revert this. However, without the three things from #86470 (comment) commented clearly in the code, I'm not confident I would be easily able to fix any problems with this derivation in the future.

If updating to a new version of memtest86-efi went smoothly, everything would probably be fine, but if it didn't, I'm not sure how I'd solve the problem. The first thing I'd probably try is reverting this PR (although if p7zip is no longer in nixpkgs, I imagine I'd have to revert even further back, before I was using p7zip).

edit: I should also add that memtest86-efi gets updated pretty rarely--maybe once every couple of months. So there is really no time-pressure for send a PR adding documentation.

This comment has been minimized.

Copy link
@flokli

flokli May 2, 2020

Contributor

@cdepillabout p7zip has been marked as insecure, so it can't be used anymore in a build, also not if the extracted contents are "known"/trusted.

Googling mtools ("a collection of utilities to access MS-DOS disks from GNU and Unix without mounting them"), plus the comment above ("# The following uses dd and mcopy to extract the actual EFI app from the usb image so that it can be installed directly on the hard drive.") was enough for me to understand what was going on - 7zip is able to extract from fat filesystems images (which this package made use of before), this was replaced by using mtools to extract the partition contents (image minus partition table) of that image.

How would you like to see the comment being improved further?

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 3, 2020

Member

@flokli Thanks for responding here.

I think we may be in the same boat. I also basically understand what is going on.

How would you like to see the comment being improved further?

There are three things in specific I'd like to see in a comment in the code (taken from #86470 (comment)):

  • Why is dd skipping the first 2048 sectors? (If you're familiar with Linux at all, it is easy to guess that this is skipping the partition table in the image, but it would be nice to have this in a comment. Otherwise, 2048 looks like a magic number.)
  • The output of dd that we are passing to mtool actually contains two separate partitions. Why it is safe to include both partitions in the thing we pass to mtool? Is it documented somewhere that mtool will ignore the second partition? (This is my main concern, and the main thing I'd like to see documented. Although, like I said above, if it is too much work to confirm that mtool does the right thing here in all cases, I'd be fine with just a comment explaining that it appears to work, and a warning for people looking at this in the future that we are relying on this behavior.)
  • A short comment about what mtool is doing. I had never seen mtool before. (This is the least important, and like you say, can be figured out from a quick google. I don't think having a one-sentence comment here hurts though.)

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout May 3, 2020

Member

@flokli In case you didn't see it, there is a PR from @emilazy adding some of these things to the documentation, and making the derivation a little more robust: #86623.

@emilazy emilazy mentioned this pull request May 3, 2020
3 of 10 tasks complete
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

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