Re: [PATCH 3/3] configure.ac: check for HMAC_CTX_cleanup

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 7, 2015, 21:46 UTC
Message-ID: <CAPig+cQ5dibu7ETWLfO5zbeeuVu2rcTDP3ghN9pt1cLrw141uw@mail.gmail.com>
In-Reply-To: <1420662236-27593-4-git-send-email-reubenhwk@gmail.com>

On Wed, Jan 7, 2015 at 3:23 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:

> OpenSSL version 0.9.6b and before defined the function HMAC_cleanup.
> Newer versions define HMAC_CTX_cleanup.  Check for HMAC_CTX_cleanup and
> fall back to HMAC_cleanup when the newer function is missing.

Missing sign-off.

Overall, these patches are nicely improved from the previous round. A few more comments below...

Show 17 quoted lines

> ---
>  Makefile          | 3 +++
>  configure.ac      | 7 +++++++
>  git-compat-util.h | 3 +++
>  3 files changed, 13 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index af551a0..d3c2b58 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1059,6 +1059,9 @@ ifndef NO_OPENSSL
>         ifdef NEEDS_CRYPTO_WITH_SSL
>                 OPENSSL_LIBSSL += -lcrypto
>         endif
> +       ifdef NO_HMAC_CTX_CLEANUP
> +               BASIC_CFLAGS += -DNO_HMAC_CTX_CLEANUP
> +       endif

You need to document this new Makefile variable (NO_HMAC_CTX_CLEANUP) at the top of Makefile (as mentioned in my previous review[1]).

[1]: http://article.gmane.org/gmane.comp.version-control.git/261631

Show 17 quoted lines

>  else
>         BASIC_CFLAGS += -DNO_OPENSSL
>         BLK_SHA1 = 1
> diff --git a/configure.ac b/configure.ac
> index 424dec5..c282663 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -923,6 +923,13 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define NO_HMAC_CTX_CLEANUP=YesPlease if HMAC_CTX_cleanup is missing.
> +AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
> +       [NO_HMAC_CTX_CLEANUP=],
> +       [NO_HMAC_CTX_CLEANUP=YesPlease],
> +       [])
> +GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP])

It is customary to drop empty trailing arguments in m4.

Also, you can simplify this entire check to:

    AC_CHECK_LIB([crypto], [HMAC_CTX_cleanup],
        [], [GIT_CONF_SUBST([NO_HMAC_CTX_CLEANUP], [YesPlease])])

>  # Define HAVE_CLOCK_GETTIME=YesPlease if clock_gettime is available.
>  GIT_CHECK_FUNC(clock_gettime,
>  [HAVE_CLOCK_GETTIME=YesPlease],