Re: [PATCH v3 3/5] elf: Make the tunable struct definition internal only

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 12/01/2021 14:22, Szabolcs Nagy via Libc-alpha wrote:
> The representation of the tunables including type information and
> the tunable list structure are only used in the implementation not
> in the tunables api that is exposed to usage within glibc.
> 
> This patch moves the representation related definitions into the
> existing dl-tunable-types.h and uses that only for implementation.
> 
> The tunable callback and related types are moved to dl-tunables.h
> because they are part of the tunables api.
> 
> This reduces the details exposed in the tunables api so the internals
> are easier to change.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-tunable-types.h   | 42 ++++++++++++++++++++++++++++++----------
>  elf/dl-tunables.h        | 35 ++++++++-------------------------
>  scripts/gen-tunables.awk |  4 +++-
>  3 files changed, 43 insertions(+), 38 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 8f6a383dcc..05d4958e1c 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -1,4 +1,4 @@
> -/* Tunable type information.
> +/* Internal representation of tunables.
>  
>     Copyright (C) 2016-2021 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
> @@ -18,8 +18,14 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #ifndef _TUNABLE_TYPES_H_
> -# define _TUNABLE_TYPES_H_
> +#define _TUNABLE_TYPES_H_
> +
> +/* Note: This header is included in the generated dl-tunables-list.h and
> +   only used internally in the tunables implementation in dl-tunables.c.  */
> +
> +#include <stdbool.h>
>  #include <stddef.h>
> +#include <stdint.h>
>  
>  typedef enum
>  {
> @@ -36,14 +42,6 @@ typedef struct
>    int64_t max;
>  } tunable_type_t;
>  
> -typedef union
> -{
> -  int64_t numval;
> -  const char *strval;
> -} tunable_val_t;
> -
> -typedef void (*tunable_callback_t) (tunable_val_t *);
> -
>  /* Security level for tunables.  This decides what to do with individual
>     tunables for AT_SECURE binaries.  */
>  typedef enum
> @@ -58,5 +56,29 @@ typedef enum
>    TUNABLE_SECLEVEL_NONE = 2,
>  } tunable_seclevel_t;
>  
> +/* A tunable.  */
> +struct _tunable
> +{
> +  const char *name;			/* Internal name of the tunable.  */
> +  tunable_type_t type;			/* Data type of the tunable.  */
> +  tunable_val_t val;			/* The value.  */
> +  bool initialized;			/* Flag to indicate that the tunable is
> +					   initialized.  */
> +  tunable_seclevel_t security_level;	/* Specify the security level for the
> +					   tunable with respect to AT_SECURE
> +					   programs.  See description of
> +					   tunable_seclevel_t to see a
> +					   description of the values.
> +
> +					   Note that even if the tunable is
> +					   read, it may not get used by the
> +					   target module if the value is
> +					   considered unsafe.  */
> +  /* Compatibility elements.  */
> +  const char *env_alias;		/* The compatibility environment
> +					   variable name.  */
> +};
> +
> +typedef struct _tunable tunable_t;
>  
>  #endif

Ok.

> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index 518342a300..1773c7e254 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -21,8 +21,6 @@
>  #ifndef _TUNABLES_H_
>  #define _TUNABLES_H_
>  
> -#include <stdbool.h>
> -
>  #if !HAVE_TUNABLES
>  static inline void
>  __always_inline
> @@ -31,34 +29,17 @@ __tunables_init (char **unused __attribute__ ((unused)))
>    /* This is optimized out if tunables are not enabled.  */
>  }
>  #else
> -
> +# include <stdbool.h>
>  # include <stddef.h>
> -# include "dl-tunable-types.h"
> +# include <stdint.h>
>  
> -/* A tunable.  */
> -struct _tunable
> +typedef union
>  {
> -  const char *name;			/* Internal name of the tunable.  */
> -  tunable_type_t type;			/* Data type of the tunable.  */
> -  tunable_val_t val;			/* The value.  */
> -  bool initialized;			/* Flag to indicate that the tunable is
> -					   initialized.  */
> -  tunable_seclevel_t security_level;	/* Specify the security level for the
> -					   tunable with respect to AT_SECURE
> -					   programs.  See description of
> -					   tunable_seclevel_t to see a
> -					   description of the values.
> -
> -					   Note that even if the tunable is
> -					   read, it may not get used by the
> -					   target module if the value is
> -					   considered unsafe.  */
> -  /* Compatibility elements.  */
> -  const char *env_alias;		/* The compatibility environment
> -					   variable name.  */
> -};
> -
> -typedef struct _tunable tunable_t;
> +  int64_t numval;
> +  const char *strval;
> +} tunable_val_t;
> +
> +typedef void (*tunable_callback_t) (tunable_val_t *);
>  
>  /* Full name for a tunable is top_ns.tunable_ns.id.  */
>  # define TUNABLE_NAME_S(top,ns,id) #top "." #ns "." #id

Ok.

> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index 622199061a..cda12ef62e 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -156,8 +156,10 @@ END {
>    }
>    print "} tunable_id_t;\n"
>  
> -  # Finally, the tunable list.
>    print "\n#ifdef TUNABLES_INTERNAL"
> +  # Internal definitions.
> +  print "# include \"dl-tunable-types.h\""
> +  # Finally, the tunable list.
>    print "static tunable_t tunable_list[] attribute_relro = {"
>    for (tnm in types) {
>      split (tnm, indices, SUBSEP);
> 

Ok.