Re: [PATCH 4/4] elfcompress: Replace cleanup() with label

[Mark Wielaard <mark@klomp.org>]

Hi Timm,

On Wed, 2021-02-17 at 09:45 +0100, Timm Bäder via Elfutils-devel wrote:
> From: Timm Bäder <tbaeder@redhat.com>
> 
> Get rid of a nested function this way.

It would be nice to get a ChangeLog entry with this.

> diff --git a/src/elfcompress.c b/src/elfcompress.c
> index 65e28f0e..ba08e73f 100644
> --- a/src/elfcompress.c
> +++ b/src/elfcompress.c
> @@ -298,47 +298,13 @@ process_file (const char *fname)
>  
>    /* How many sections are we talking about?  */
>    size_t shnum = 0;
> -
> -  int cleanup (int res)
> -  {
> -    elf_end (elf);
> -    close (fd);
> -
> -    elf_end (elfnew);
> -    close (fdnew);
> -
> -    if (fnew != NULL)
> -      {
> -	unlink (fnew);
> -	free (fnew);
> -	fnew = NULL;
> -      }
> -
> -    free (snamebuf);
> -    if (names != NULL)
> -      {
> -	dwelf_strtab_free (names);
> -	free (scnstrents);
> -	free (symstrents);
> -	free (namesbuf);
> -	if (scnnames != NULL)
> -	  {
> -	    for (size_t n = 0; n < shnum; n++)
> -	      free (scnnames[n]);
> -	    free (scnnames);
> -	  }
> -      }
> -
> -    free (sections);
> -
> -    return res;
> -  }
> +  int res = 0;

If you are going to use a goto to the cleanup it makes sense to
initialize res to -1 here. Since till you get to the end, the result
will always be -1. And then you only need one (cleanup) label at the
end.

> @@ -1294,14 +1260,48 @@ process_file (const char *fname)
>      if (rename (fnew, fname) != 0)
>        {
>  	error (0, errno, "Couldn't rename %s to %s", fnew, fname);
> -	return cleanup (-1);
> +	goto error;
>        }
>  
>    /* We are finally done with the new file, don't unlink it now.  */
>    free (fnew);
>    fnew = NULL;
> +  goto cleanup;
> +
> +error:
> +  res = -1;

And then here instead of the goto cleanup and having an error label
simply set res = 0 and fall through to the cleanup label.

> +cleanup:
> +  elf_end (elf);
> +  close (fd);
>  
> -  return cleanup (0);
> +  elf_end (elfnew);
> +  close (fdnew);
> +
> +  if (fnew != NULL)
> +    {
> +      unlink (fnew);
> +      free (fnew);
> +      fnew = NULL;
> +    }
> +
> +  free (snamebuf);
> +  if (names != NULL)
> +    {
> +      dwelf_strtab_free (names);
> +      free (scnstrents);
> +      free (symstrents);
> +      free (namesbuf);
> +      if (scnnames != NULL)
> +	{
> +	  for (size_t n = 0; n < shnum; n++)
> +	    free (scnnames[n]);
> +	  free (scnnames);
> +	}
> +    }
> +
> +  free (sections);
> +  return res;
>  }

Cheers,

Mark