[1/25] Remove nested functions: crypt/md5-crypt.c
diff mbox

Message ID CAGQ9bdzqT1EyXYMwACrHpPU=vPjM_b72LJRjb7BW_OzJRXG3bw@mail.gmail.com
State New
Headers show

Commit Message

Konstantin Serebryany May 20, 2014, 12:56 p.m. UTC
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)

2014-05-20 Kostya Serebryany <konstantin.s.serebryany@gmail.com>
        * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function.
        (b64_from_24bit): New function.


    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);

Comments

Siddhesh Poyarekar May 20, 2014, 1:13 p.m. UTC | #1
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
Joseph Myers May 20, 2014, 2:32 p.m. UTC | #2
On Tue, 20 May 2014, Konstantin Serebryany wrote:

> (If approved, this will be my first commit to glibc, so some more
> actions may be required)

I don't see you in copyright.list - are you covered by a corporate 
copyright assignment?  Even if individual patches in this series don't 
have much copyrightable creativity, the series as a whole still may.

(Also: anyone working on this should *not* look at Qualcomm's patches for 
building glibc for Hexagon with Clang, as Qualcomm does not have a 
copyright assignment.)
Konstantin Serebryany May 20, 2014, 2:39 p.m. UTC | #3
On Tue, May 20, 2014 at 6:32 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Tue, 20 May 2014, Konstantin Serebryany wrote:
>
>> (If approved, this will be my first commit to glibc, so some more
>> actions may be required)
>
> I don't see you in copyright.list - are you covered by a corporate
> copyright assignment?
Err. Most likely. (My other e-mail is kcc@google.com)

> Even if individual patches in this series don't
> have much copyrightable creativity, the series as a whole still may.
>
> (Also: anyone working on this should *not* look at Qualcomm's patches for
> building glibc for Hexagon with Clang, as Qualcomm does not have a
> copyright assignment.)
Never heard of such :)

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers May 20, 2014, 2:53 p.m. UTC | #4
On Tue, 20 May 2014, Konstantin Serebryany wrote:

> On Tue, May 20, 2014 at 6:32 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
> > On Tue, 20 May 2014, Konstantin Serebryany wrote:
> >
> >> (If approved, this will be my first commit to glibc, so some more
> >> actions may be required)
> >
> > I don't see you in copyright.list - are you covered by a corporate
> > copyright assignment?
> Err. Most likely. (My other e-mail is kcc@google.com)

Yes, there's a corporate assignment from Google for all GNU packages, 
dated 2007-03-15.

Patch
diff mbox

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,
+                            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