pvgrub_image: add package by michalpalka · Pull Request #25724 · NixOS/nixpkgs

michalpalka · 2017-05-12T06:42:07Z

Add a package containing a pvgrub image for xen generated from grub2

Motivation for this change

This package provides a pvgrub image for booting PV guests in xen. Currently, xen in NixOS also provides pygrub, but it can be used only to boot guests that use older grub utilising menu.lst, and fails with new NixOS guests, for example.

I would appreciate if someone takes a look at this before merging.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-05-12T06:42:09Z

@michalpalka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @wkennington, @dezgeg and @edolstra to be potential reviewers.

0xABAB · 2017-07-01T17:47:40Z

pkgs/tools/misc/grub/pvgrub_image/default.nix

+    # if we include it.
+    grub-mkimage -O ${efiSystemsBuild.${stdenv.system}.target}-xen -c grub-bootstrap.cfg \
+      -m memdisk.tar -o grub-${efiSystemsBuild.${stdenv.system}.target}-xen.bin \
+      `ls ${grub2_xen}/lib/grub/${efiSystemsBuild.${stdenv.system}.target}-xen/ |grep 'mod''$'|grep -v '^all_video\.mod''$'`


Use find, not ls.

Also don't use back ticks, but $().

And add ""s pretty much everywhere.

I have put quotes around all file names with variables in them, and addressed the other changes. Please let me know if this is what you wanted.

Actually, I had to revert back to using ls instead of find, as find produces full paths instead of just file names. If there is some specific reason not to use ls, please let me know. Otherwise I would prefer to stay with ls, as using find will require adding one more processing step to the pipeline.

0xABAB · 2017-07-01T17:48:19Z

pkgs/tools/misc/grub/pvgrub_image/default.nix

+      -m memdisk.tar -o grub-${efiSystemsBuild.${stdenv.system}.target}-xen.bin \
+      `ls ${grub2_xen}/lib/grub/${efiSystemsBuild.${stdenv.system}.target}-xen/ |grep 'mod''$'|grep -v '^all_video\.mod''$'`
+    mkdir -p $out/lib/grub-xen
+    cp grub-${efiSystemsBuild.${stdenv.system}.target}-xen.bin $out/lib/grub-xen/


The braces look unbalanced.

The variables also need to be quoted.

Could you point at which braces are unbalanced here? I can't see any.

I have put quotes around file names here too.

0xABAB · 2017-07-01T17:50:05Z

pkgs/tools/misc/grub/pvgrub_image/configs/grub.cfg

@@ -0,0 +1,9 @@
+if search -s -f /boot/grub/grub.cfg ; then
+	echo "Reading (${root})/boot/grub/grub.cfg"


The parentheses and ${root} might make sense in a larger context, but they don't to me in this small context.

The parentheses are here to match Grub's config file syntax. In fact, the file has been taken from here. If you think that there is a good reason to remove them, then of course I can do it.

Why are there two paths to read grub.cfg from? One is /boot/grub/grub.cfg. If we only use one, eliminate one please.

Regarding the parentheses, adding the comment
The parentheses around ${root} here to match Grub's config file syntax to the source code would work wonders.

My understanding is that it is a convenience feature, which allows you to have a separate /boot partition and still use the same pre-built pvgrub image, as explained on the Xen project wiki:

Bootstrapping into this more complex grub.cfg allows us to cope with guests which have a separate /boot partition by searching for a file named /boot/grub/grub.cfg or /grub/grub.cfg on any partition. Note that ${root} is set by the search command and should be entered literally, not substituted while creating this file.

Since the Xen project wiki is advocating for this solution, I would recommend keeping it.

I have added the comment about parentheses.

0xABAB · 2017-07-01T17:53:12Z

pkgs/tools/misc/grub/pvgrub_image/default.nix

+    description = "PvGrub image for use for booting PV Xen guests";
+
+    longDescription =
+      '' This package provides a PvGrub image for booting PV Xen guests


It's a long description, so just state Paravirtualization (PV).

michalpalka · 2017-07-05T11:35:01Z

Thanks for the comments! I have amended the commit to address them.

0xABAB · 2017-07-05T11:50:29Z

pkgs/tools/misc/grub/pvgrub_image/default.nix

https://wiki.xenproject.org/wiki/Xen_EFI lists all_video.mod in one particular configuration, so your statement doesn't seem precise enough.

The comment is based on my observation that pvgrub fails when we include that module. The website that you linked to concerns booting Xen itself using EFI, which requires compiling grub as an EFI loader. The all_video.mod module is probably working in that setup, which does not contradict what I wrote. Please let me know if I should include any clarification in the comment.

@michalpalka I like content without words like "I believe" or "probably" better than content that just guesses something.

While I understand it's hard to know everything, striving to documenting the reasons for failures makes it possible to improve things later on.

It has already been merged, which is good for continuity, but if you do have a superior understand of exactly why things happen the way they do with all_video.mod, please document it.

Add a package containing a pvgrub image for xen generated from grub2

michalpalka · 2017-07-06T08:40:23Z

Thanks a lot @0xABAB and @7c6f434c !

pSub added the 8.has: package (new) This PR adds a new package label May 14, 2017

michalpalka mentioned this pull request Jul 1, 2017

xen_4_8: init at 4.8.1 #26492

Merged

7 tasks

0xABAB reviewed Jul 1, 2017

View reviewed changes

michalpalka force-pushed the pvgrub-new branch from ef8823d to 6c48eac Compare July 5, 2017 11:16

0xABAB reviewed Jul 5, 2017

View reviewed changes

michalpalka force-pushed the pvgrub-new branch from 6c48eac to 121f0c8 Compare July 5, 2017 12:10

pvgrub_image: add package

55208cc

Add a package containing a pvgrub image for xen generated from grub2

michalpalka force-pushed the pvgrub-new branch from 121f0c8 to 55208cc Compare July 5, 2017 12:23

7c6f434c merged commit d38d3d1 into NixOS:master Jul 5, 2017

		@@ -0,0 +1,9 @@
		if search -s -f /boot/grub/grub.cfg ; then
		echo "Reading (${root})/boot/grub/grub.cfg"

Uh oh!

Conversation

michalpalka commented May 12, 2017

Motivation for this change

Things done

Uh oh!

mention-bot commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalpalka Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalpalka commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalpalka commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michalpalka Jul 5, 2017 •

edited

Loading