Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Avnish Chouhan <avnish@linux.ibm.com>]

On 2025-09-16 10:28, Michael Chang wrote:
> Hi Avnish,
> 
> Thanks for the review. Please see my comments below.
> 
> On Mon, Sep 15, 2025 at 06:37:05PM +0530, Avnish Chouhan wrote:
>> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
>> >
>> > Message: 2
>> > Date: Mon, 15 Sep 2025 17:08:40 +0800
>> > From: Michael Chang <mchang@suse.com>
>> > To: The development of GNU GRUB <grub-devel@gnu.org>
>> > Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
>> > 	<mlewando@redhat.com>
>> > Subject: [PATCH v2 1/9] util/grub-editenv: add basic structures and
>> > 	probe call for external envblk
>> > Message-ID: <20250915090848.131937-2-mchang@suse.com>
>> >
>> > This patch prepares for using an environment block stored in a reserved
>> > area of the filesystem. It adds a constant ENV_BTRFS_OFFSET at 256 KiB
>> > in the Btrfs header. It also introduces the fs_envblk_spec and fs_envblk
>> > structures together with the probe_fs_envblk function to identify the
>> > root filesystem and to avoid configurations that involve diskfilter or
>> > cryptodisk.
>> >
>> > The probe is only invoked when grub-editenv is working on the default
>> > environment file path. This restriction ensures that probing and
>> > possible raw device access are not triggered for arbitrary user supplied
>> > paths, but only for the standard grubenv file. In that case the code
>> > checks if the filename equals DEFAULT_ENVBLK_PATH and then calls
>> > probe_fs_envblk with fs_envblk_spec. The result is stored in the global
>> > fs_envblk handle. At this stage the external environment block is only
>> > detected and recorded, and the behavior of grub editenv is unchanged.
>> >
>> > Signed-off-by: Michael Chang <mchang@suse.com>
> 
> Well xstrdup() and other x* functions like xmalloc() and xcalloc()
> already handle the NULL check, so we do not need to add repetitive NULL
> checks and error handling in the code. This makes the code more concise
> and less verbose IMHO.
> 
> 
> I hope the clarification makes sense to you.
> 
> Thanks,
> Michael
> 
>> 
>> Regards,
>> Avnish Chouhan

Hi Michael,
Thank you so much for your clarification!

In my opinion, using grub_*() would be better instead of x*(). My 
suggestion here was not about adding a NULL check, it was about "we can 
use grub_*() and add a NULL check instead of using x*()". Using grub_*() 
would be safe!

Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel