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, [email protected] wrote:
>
> Message: 2
> Date: Mon, 15 Sep 2025 17:08:40 +0800
> From: Michael Chang <[email protected]>
> To: The development of GNU GRUB <[email protected]>
> Cc: Neal Gompa <[email protected]>, Marta Lewandowska
> <[email protected]>
> Subject: [PATCH v2 1/9] util/grub-editenv: add basic structures and
> probe call for external envblk
> Message-ID: <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
