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
Conversation
Follow-up to #86470. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
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. |
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 After playing around with this for a while, I figured out that passing the I don't think this |
15172cf
to
3ae05b6
Compare
Motivation for this change
Uses
sfdisk(1)
+jq(1)
rather than hardcoding a partition offset, and avoids an intermediatedd(1)
step by passing the offset directly to mtools.Hopefully the derivation is less obscure / better documented now.
cc @flokli @cdepillabout
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)