On Tue, Feb 18, 2020 at 11:10:07AM -0800, Paul Dagnelie wrote: > Hey all, I previously discussed my concept for this patch in my email > https://lists.gnu.org/archive/html/grub-devel/2020-01/msg00004.html . > I'm pleased to announce that I've gotten it into a working state and > it is ready to review. There are a number of changes here, which I > will break down below. > > First, the concept of "special files" is introduced into grub. These > are files that are manipulated using different functions from the > remainder of the filesystem. A filesystem advertises its support for > these files by filling in new entries in the grub_fs struct. > > Second, the loadenv command was modified to use the special file > functions of the root filesystem if they are detected. If that > happens, these functions are used to open, read, and write to the > loadenv file instead of the normal grub file functions. A variable > was also added that allows the user to force a file to be used instead > of the special files, or vice versa. > > Third, the grub-editenv command was modified to teach it to probe the > root filesystem and then check in an array of structures that contain
Why "root" not "boot"? > functions that will read and modify the filesystem's special file in > userland. These functions are very similar to normal read and write > functions, and so don't result in a very different code flow. > > Finally, the GRUB ZFS logic was modified to have new special_file > functions. These functions manipulate the pad2 area of the ZFS label, > which was previously unused. It now stores a version number, the raw > contents of the grubenv file, and an embedded checksum for integrity > purposes. GRUB was taught to read and verify these areas, and also to > modify them, computing the embeddded checksum appropriately. In > addition, if GRUB is build with libzfs support, an entry is added to > the grub-editenv table that points GRUB to the appropriate libzfs > functions, and init and fini functions are built into grub-editenv > itself. > > Future work: > * In the ZFS code could store a packed nvlist instead of a raw file, > but this would involve further changes to the grub environment file > code and was deferred for when it would be more useful and important. > * For the special files code, there is only one special file allowed > per filesystem, but a specification interface (using either an enum or > a set of names) could be added in the future. > * Other filesystem support was not built into this change, but > extensibility was a primary goal, so adding support for btrfs or other > filesystems should be relatively straightforward. > * I did not implement the proposed UEFI variable support, but I did > develop this with that future in mind, so adding it should be a > relatively simple extension of these changes. > > This patch has been tested on systems with and without ZFS as the boot > filesystem, and built with and without ZFS libraries installed. It > worked in each case I tried, including with manual corruption applied > to the ZFS labels to force GRUB to read from the other label. This > was tested in conjunction with > https://github.com/zfsonlinux/zfs/pull/10009 , the ZoL patch that > enables ZFS to read from and write to the padding areas we reserved > for the data. > > Let me know if you have any feedback, I'm sure there's some stuff I'm > doing wrong or in a non-idiomatic fashion. In addition, if you'd like > this patch divided into multiple smaller patches, I would also be > happy to oblige. Yes, please split the patch into smaller patches. Please do one logical change per patch. I quickly went through the patch and pointed things which I spotted at first sight. I will provide more comments when you split the patch into separate patches. Next time please CC following people too: javi...@redhat.com, maciej.pijanow...@3mdeb.com and piotr.k...@3mdeb.com. > grub-core/commands/loadenv.c | 122 +++++++++++++++++--- > grub-core/fs/zfs/zfs.c | 131 +++++++++++++++++++-- > grub-core/kern/file.c | 74 ++++++++---- > grub-core/lib/envblk.c | 2 +- > include/grub/file.h | 10 ++ > include/grub/fs.h | 12 +- > include/grub/lib/envblk.h | 2 + > include/grub/util/install.h | 3 + > include/grub/util/libzfs.h | 5 + > include/grub/zfs/vdev_impl.h | 33 +++--- > util/editenv.c | 26 +++-- > util/grub-editenv.c | 267 > +++++++++++++++++++++++++++++++++++++++++-- > 12 files changed, 603 insertions(+), 84 deletions(-) > > diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c > index 3fd664aac..3553c2f94 100644 > --- a/grub-core/commands/loadenv.c > +++ b/grub-core/commands/loadenv.c > @@ -79,6 +79,94 @@ open_envblk_file (char *filename, > return file; > } > > +static grub_file_t > +open_envblk_block (grub_fs_t fs, grub_device_t dev, enum grub_file_type type) > +{ > + if (!fs->fs_special_open) > + return NULL; > + return grub_file_special_open(fs, dev, type); I think more natural would be... if (fs->fs_special_open) return grub_file_special_open (fs, dev, type); else return NULL; ...and it seems to me that "fs_special_open" name is too generic. What about "fs_envblk_open" or "fs_ext_data_open" (extra data) or something like that? Same for other functions... > +} > + > +static grub_file_t > +open_envblk (grub_extcmd_context_t ctxt, enum grub_file_type type) > +{ > + struct grub_arg_list *state = ctxt->state; > + grub_file_t file = NULL; > + grub_device_t device = NULL; > + const char *force; > + grub_fs_t fs = NULL; > + char *filename = (state[0].set) ? state[0].arg : 0; I think that you can drop parenthesis here. And please use NULL instead of 0. > + force = grub_env_get ("grubenv_force"); "grubenv_src" instead of "grubenv_force"? [...] > diff --git a/include/grub/fs.h b/include/grub/fs.h > index 302e48d4b..99a1bf71e 100644 > --- a/include/grub/fs.h > +++ b/include/grub/fs.h > @@ -65,7 +65,7 @@ struct grub_fs > grub_err_t (*fs_open) (struct grub_file *file, const char *name); > > /* Read LEN bytes data from FILE into BUF. */ > - grub_ssize_t (*fs_read) (struct grub_file *file, char *buf, grub_size_t > len); > + grub_ssize_t (*fs_read) (struct grub_file *file, char *buf, grub_size_t > len); > > /* Close the file FILE. */ > grub_err_t (*fs_close) (struct grub_file *file); > @@ -96,6 +96,16 @@ struct grub_fs > /* Whether blocklist installs have a chance to work. */ > int blocklist_install; > #endif > + > + /* The special file functions are defined on filesystems that need to > + handle grub-writable files in a special way. This is most commonly the > + case for CoW filesystems like btrfs and ZFS. The normal read and close > + functions should detect that they are being called on a special file and > + act appropriately. */ This comment is incorrectly formatted. Please read this [1]. Daniel [1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
