Re: [PATCH] ZFS/other CoW FS save_env support

[Paul Dagnelie]

On Mon, Feb 24, 2020 at 3:03 AM Daniel Kiper <address@hidden> wrote:
>
>
> Why "root" not "boot"?
That was a typo on my part; the code uses grub_guess_root_device to
find the devices backing the default grub directory, but in most
configurations, this should attempt to locate the boot filesystem
instead of the root. I was uncertain of a better way to consistently
determine the boot filesystem, and this portion of the code was copied
from another GRUB patch
(https://build.opensuse.org/package/view_file/openSUSE:Factory/grub2/grub2-grubenv-in-btrfs-header.patch?expand=1).

>
> 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: address@hidden,
> address@hidden and address@hidden.
Understood! I will post an updated version hopefully today or tomorrow.

>
> I think that you can drop parenthesis here. And please use NULL instead of 0.
Will do. In general, this was one of my questions about writing new
code in this code base. There are several things where I decided to go
with consistency with surrounding code instead of what would commonly
be preferred in modern coding standards or by the style guide (see
also, the block comment style you mentioned further down). Is there a
preference in this codebase against consistency when other
considerations are also relevant?



-- 
Paul Dagnelie