Re: [PATCH v1 1/2] efi: add grub_variable_with_attributes

[Daniel Kiper]

On Mon, Jan 16, 2023 at 12:40:52PM +0100, Oliver Steffen wrote:
> Add a function to the EFI module that allows setting EFI variables
> with specific attributes.
>
> This is useful for marking variables as volatile, for example.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> ---
>  grub-core/kern/efi/efi.c | 19 +++++++++++++------
>  include/grub/efi/efi.h   |  6 ++++++
>  2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index cf49d6357..96fa96c15 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -201,8 +201,8 @@ grub_efi_set_virtual_address_map (grub_efi_uintn_t 
> memory_map_size,
>  }
>
>  grub_err_t
> -grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
> -                   void *data, grub_size_t datasize)
> +grub_efi_set_variable_with_attributes(const char *var, const grub_efi_guid_t 
> *guid,

Please add missing space before "(".

> +                   void *data, grub_size_t datasize, grub_uint32_t 
> attributes)

s/grub_uint32_t attributes/grub_efi_uint32_t attributes/?

>  {
>    grub_efi_status_t status;
>    grub_efi_runtime_services_t *r;
> @@ -219,10 +219,7 @@ grub_efi_set_variable(const char *var, const 
> grub_efi_guid_t *guid,
>
>    r = grub_efi_system_table->runtime_services;
>
> -  status = efi_call_5 (r->set_variable, var16, guid,
> -                    (GRUB_EFI_VARIABLE_NON_VOLATILE
> -                     | GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
> -                     | GRUB_EFI_VARIABLE_RUNTIME_ACCESS),
> +  status = efi_call_5 (r->set_variable, var16, guid, attributes,
>                      datasize, data);
>    grub_free (var16);
>    if (status == GRUB_EFI_SUCCESS)
> @@ -231,6 +228,16 @@ grub_efi_set_variable(const char *var, const 
> grub_efi_guid_t *guid,
>    return grub_error (GRUB_ERR_IO, "could not set EFI variable `%s'", var);
>  }
>
> +grub_err_t
> +grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,

Again, missing space before "("...

> +                   void *data, grub_size_t datasize)
> +{
> +  return grub_efi_set_variable_with_attributes (var, guid, data, datasize,
> +                     GRUB_EFI_VARIABLE_NON_VOLATILE
> +                     | GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
> +                     | GRUB_EFI_VARIABLE_RUNTIME_ACCESS);
> +}
> +
>  grub_efi_status_t
>  grub_efi_get_variable_with_attributes (const char *var,
>                                      const grub_efi_guid_t *guid,
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index e61272de5..d7bf0765b 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -86,6 +86,12 @@ grub_efi_status_t EXPORT_FUNC (grub_efi_get_variable) 
> (const char *variable,
>                                                      grub_size_t 
> *datasize_out,
>                                                      void **data_out);
>  grub_err_t
> +EXPORT_FUNC (grub_efi_set_variable_with_attributes) (const char *var,
> +                                  const grub_efi_guid_t *guid,
> +                                  void *data,
> +                                  grub_size_t datasize,
> +             grub_uint32_t attribute);
> +grub_err_t
>  EXPORT_FUNC (grub_efi_set_variable) (const char *var,
>                                    const grub_efi_guid_t *guid,
>                                    void *data,

Otherwise patch LGTM...

Daniel