Re: [PATCH 1/3] link_map: Pull release_buffer() into file scope

[Mark Wielaard <mark@klomp.org>]

Hi Timm,

On Tue, 2020-12-01 at 09:38 +0100, Timm Bäder via Elfutils-devel wrote:
> From: Timm Bäder <tbaeder@redhat.com>
> 
> Get rid of another nested function

It is missing a ChangeLog entry.

> diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
> index 29307c74..5c39c631 100644
> --- a/libdwfl/link_map.c
> +++ b/libdwfl/link_map.c
> @@ -225,6 +225,18 @@ addrsize (uint_fast8_t elfclass)
>    return elfclass * 4;
>  }
>  
> +static inline int
> +release_buffer (Dwfl *dwfl,
> +                Dwfl_Memory_Callback *memory_callback, void *memory_callback_arg,
> +                void **buffer, size_t *buffer_available,
> +                int result)
> +{
> +  if (buffer != NULL)
> +    (void) (*memory_callback) (dwfl, -1, buffer, buffer_available, 0, 0,
> +                               memory_callback_arg);
> +  return result;
> +}

Note that this changes the semantics slightly. Because you now take the
address of the buffer variable before checking it is NULL (it now never
is). Also note that the result is not always used, so it might be
cleaner to not pass it around. It is IMHO better to fully inline it.

> @@ -249,13 +261,6 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>  
>    void *buffer = NULL;
>    size_t buffer_available = 0;
> -  inline int release_buffer (int result)
> -  {
> -    if (buffer != NULL)
> -      (void) (*memory_callback) (dwfl, -1, &buffer, &buffer_available, 0, 0,
> -                                memory_callback_arg);
> -    return result;
> -  }
>  
>    GElf_Addr addrs[4];
>    inline bool read_addrs (GElf_Addr vaddr, size_t n)
> [...]
> @@ -304,7 +310,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>    }
>  
>    if (unlikely (read_addrs (read_vaddr, 1)))
> -    return release_buffer (-1);
> +    release_buffer (dwfl, memory_callback, memory_callback_arg,
> +                    &buffer, &buffer_available, -1);
> +

Note that here the result is used, but the change doesn't use it
anymore and so doesn't return early but just tries to carry on after an
error occurred.

Cheers,

Mark