Re: [PATCH 1/25] Remove nested functions: crypt/md5-crypt.c

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Tue, May 20, 2014 at 04:56:27PM +0400, Konstantin Serebryany wrote:
> Hi,
> 
> This patch is the first in the series of patches that remove nested
> functions from glibc.
> Rationale: nested functions is a non-standard language feature;
> removing nested functions
> will allow to compile glibc with compilers other than GCC and thus
> benefit from other compilers
> and code analysis tools.
> Discussed previously here:
> https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html
> No functionality change intended.
> 
> (If approved, this will be my first commit to glibc, so some more
> actions may be required)

I believe initially you'll need one of us to commit for you, which we
will do once the patch is found suitable.  You may want to review the
contribution checklist as well:

http://sourceware.org/glibc/wiki/Contribution%20checklist

> 
> 2014-05-20 Kostya Serebryany <konstantin.s.serebryany@gmail.com>

Two spaces between the date and name, and name and email.  An extra
newline between the ChangeLog content and header.

>         * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function.
>         (b64_from_24bit): New function.
> 
> 
> diff --git a/crypt/md5-crypt.c b/crypt/md5-crypt.c
> index d1b92d7..29f66ce 100644
> --- a/crypt/md5-crypt.c
> +++ b/crypt/md5-crypt.c
> @@ -88,6 +88,19 @@ extern char *__md5_crypt_r (const char *key, const
> char *salt,
>                             char *buffer, int buflen);
>  extern char *__md5_crypt (const char *key, const char *salt);
> 
> +static void b64_from_24bit (char **cp, int *buflen,

'static void' should be on a separate line on its own.

> +                            unsigned int b2, unsigned int b1, unsigned int b0,
> +                            int n)
> +{
> +  unsigned int w = (b2 << 16) | (b1 << 8) | b0;
> +  while (n-- > 0 && *buflen > 0)
> +    {
> +      *(*cp)++ = b64t[w & 0x3f];
> +      --*buflen;
> +      w >>= 6;
> +    }
> +}
> +
> 
>  /* This entry point is equivalent to the `crypt' function in Unix
>     libcs.  */
> @@ -268,24 +281,18 @@ __md5_crypt_r (key, salt, buffer, buflen)
>        --buflen;
>      }
> 
> -  void b64_from_24bit (unsigned int b2, unsigned int b1, unsigned int b0,
> -                      int n)
> -  {
> -    unsigned int w = (b2 << 16) | (b1 << 8) | b0;
> -    while (n-- > 0 && buflen > 0)
> -      {
> -       *cp++ = b64t[w & 0x3f];
> -       --buflen;
> -       w >>= 6;
> -      }
> -  }
> -
> -  b64_from_24bit (alt_result[0], alt_result[6], alt_result[12], 4);
> -  b64_from_24bit (alt_result[1], alt_result[7], alt_result[13], 4);
> -  b64_from_24bit (alt_result[2], alt_result[8], alt_result[14], 4);
> -  b64_from_24bit (alt_result[3], alt_result[9], alt_result[15], 4);
> -  b64_from_24bit (alt_result[4], alt_result[10], alt_result[5], 4);
> -  b64_from_24bit (0, 0, alt_result[11], 2);
> +  b64_from_24bit (&cp, &buflen,
> +                  alt_result[0], alt_result[6], alt_result[12], 4);
> +  b64_from_24bit (&cp, &buflen,
> +                  alt_result[1], alt_result[7], alt_result[13], 4);
> +  b64_from_24bit (&cp, &buflen,
> +                  alt_result[2], alt_result[8], alt_result[14], 4);
> +  b64_from_24bit (&cp, &buflen,
> +                  alt_result[3], alt_result[9], alt_result[15], 4);
> +  b64_from_24bit (&cp, &buflen,
> +                  alt_result[4], alt_result[10], alt_result[5], 4);
> +  b64_from_24bit (&cp, &buflen,
> +                  0, 0, alt_result[11], 2);
>    if (buflen <= 0)
>      {
>        __set_errno (ERANGE);

It would be useful if you could post some kind of performance analysis
of the crypt or crypt_r to show that making this change does not cause
a significant drop in performance.  In fact, I'd be really interested
in having a microbenchmark added for this function in benchtests.
Instructions in benchtests/README should be useful but if not, please
feel free to reach out to me for help.

Siddhesh

Attachment: pgpQiBHSKbI_E.pgp
Description: PGP signature