Re: [PATCH v1 1/3] efi: filestore: don't compile when config disabled

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Nov 21, 2023 at 11:57:11PM +0000, Shantur Rathore wrote:
> Compile out filestore functions when config isn't enabled.
> 
> Signed-off-by: Shantur Rathore <i@shantur.com>
> ---
> 
>  include/efi_variable.h        | 21 ++++++++++++---------
>  lib/efi_loader/efi_var_file.c | 13 +++++++------
>  lib/efi_loader/efi_variable.c | 10 +++++++++-
>  3 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 805e6c5f1e..ca7e19d514 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -136,15 +136,6 @@ struct efi_var_file {
>  	struct efi_var_entry var[];
>  };
>  
> -/**
> - * efi_var_to_file() - save non-volatile variables as file
> - *
> - * File ubootefi.var is created on the EFI system partion.
> - *
> - * Return:	status code
> - */
> -efi_status_t efi_var_to_file(void);
> -
>  /**
>   * efi_var_collect() - collect variables in buffer
>   *
> @@ -172,6 +163,16 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>   */
>  efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE

I don't think we need this guard because any function declaration
in a header is harmless even if that function implementation is opted out.

> +/**
> + * efi_var_to_file() - save non-volatile variables as file
> + *
> + * File ubootefi.var is created on the EFI system parition.
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_var_to_file(void);
> +
>  /**
>   * efi_var_from_file() - read variables from file
>   *
> @@ -185,6 +186,8 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>   */
>  efi_status_t efi_var_from_file(void);
>  
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> +
>  /**
>   * efi_var_mem_init() - set-up variable list
>   *
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index 62e071bd83..7ceb7e3cf7 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -117,6 +117,8 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>  	return EFI_SUCCESS;
>  }
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE

Can you refactor the code further to purge all "#ifdef" stuffs from this file?

I mean:
1) move efi_var_collect/restor() to a always-compiled file, say,
   efi_var_common.c/efi_variable.c
2) modify call sites of the other functions, for example, as follows:
     if (CONFIG_IS_ENABLED(EFI_VARIABLE_STORE))
         efi_var_to_file();
   # I'm not sure why the return code is ignored everywhere.
3) modify Makefile to compile efi_var_file.c only if EFI_VARIABLE_FILE_STORE
   is enabled
4) remove "#ifdef CONFIG_EFI_VARIABLE_FILE_STORE" from efi_var_file.c

-Takahiro Akashi

> +
>  /**
>   * efi_var_to_file() - save non-volatile variables as file
>   *
> @@ -126,7 +128,6 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>   */
>  efi_status_t efi_var_to_file(void)
>  {
> -#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  	efi_status_t ret;
>  	struct efi_var_file *buf;
>  	loff_t len;
> @@ -150,11 +151,10 @@ error:
>  		log_err("Failed to persist EFI variables\n");
>  	free(buf);
>  	return ret;
> -#else
> -	return EFI_SUCCESS;
> -#endif
>  }
>  
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> +
>  efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>  {
>  	struct efi_var_entry *var, *last_var;
> @@ -198,6 +198,7 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>  	return EFI_SUCCESS;
>  }
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  /**
>   * efi_var_from_file() - read variables from file
>   *
> @@ -211,7 +212,6 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>   */
>  efi_status_t efi_var_from_file(void)
>  {
> -#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  	struct efi_var_file *buf;
>  	loff_t len;
>  	efi_status_t ret;
> @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
>  		log_err("Invalid EFI variables file\n");
>  error:
>  	free(buf);
> -#endif
>  	return EFI_SUCCESS;
>  }
> +
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index be95ed44e6..7fa444451d 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -357,8 +357,11 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>  	 * Write non-volatile EFI variables to file
>  	 * TODO: check if a value change has occured to avoid superfluous writes
>  	 */
> -	if (attributes & EFI_VARIABLE_NON_VOLATILE)
> +	if (attributes & EFI_VARIABLE_NON_VOLATILE) {
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  		efi_var_to_file();
> +#endif
> +	}
>  
>  	return EFI_SUCCESS;
>  }
> @@ -466,7 +469,12 @@ efi_status_t efi_init_variables(void)
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  	ret = efi_var_from_file();
> +#else
> +	ret = EFI_SUCCESS;
> +#endif
> +
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
> -- 
> 2.40.1
>