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

From: Michael Chang <mchang@suse.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Cc: Daniel Kiper <dkiper@net-space.pl>,
	Matthew Ahrens <mahrens@delphix.com>,
	 Brian Behlendorf <behlendorf1@llnl.gov>,
	George Wilson <george.wilson@delphix.com>,
	Sebastien Roy <sebastien.roy@delphix.com>, <javierm@redhat.com>,
	<maciej.pijanowski@3mdeb.com>, <piotr.krol@3mdeb.com>
Subject: Re: [PATCH] ZFS/other CoW FS save_env support
Date: Tue, 25 Feb 2020 18:33:09 +0800	[thread overview]
Message-ID: <20200225103309.GA14273@mazu> (raw)
In-Reply-To: <CAJ-UWOeZU1B2TRhE5B7ZbKRQhB_DQN-92WdQ1S6RmsBsRy5UNw@mail.gmail.com>

On Mon, Feb 24, 2020 at 10:30:36AM -0800, Paul Dagnelie wrote:
> On Mon, Feb 24, 2020 at 3:03 AM Daniel Kiper <dkiper@net-space.pl> 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).

I used to have the same confusion as well. But I tend to interpret that
as "root device/filesystem for grub" and thus not from the viewpoint of
a linux system. Essentially we have been using "root" as the name for
the environment variable pointing to the grub device in which /boot
resides and the concept just carried over from it to the description. 

So in my opinion it certainly not a typo but rather a common expression
mentioned by the source code. Although it would become misleading at
times when people have to take in with the linux system, I still prefer
to keep it as it wasn't that much a mistake by itself. :)  

Thanks.
Michael

> 
> >
> > 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: javierm@redhat.com,
> > maciej.pijanowski@3mdeb.com and piotr.krol@3mdeb.com.
> 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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel