[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/9] Add some autoconf helper macros for checking for libraries



On 01/10/2013 01:18 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Most checks for libraries take the same format
> 
>   * --with-libFOO=yes|no|check|/some/path  argument
>   * check for a function NNN in libFOO.so
>   * check for a header file DDD/HHH.h
>   * Define a WITH_FOO config.h symbol
>   * Define a WITH_FOO make conditional
>   * Substitute FOO_CFLAGS and FOO_LIBS make variables
>   * Print CFLAGS & LIBS summary at the end
> 

General impression - nice!

Putting on my autoconf maintainer hat: full of non-robust underquoted
potential gotchas, for example if someone ever tries to link to a
library named DNL.  I'll go ahead and point out LOTS of instances of
where you would use "m4_defn([var])" instead of plain "var" to avoid
those issues, but before you hack up code to deal with all my nits (and
realize that a lot more lines of code need the same "fixes" than just
those I called out), read this entire message.

> ---
>  m4/virt-lib.m4    | 391 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  m4/virt-result.m4 |  29 ++++
>  2 files changed, 420 insertions(+)
>  create mode 100644 m4/virt-lib.m4
>  create mode 100644 m4/virt-result.m4
> 
> diff --git a/m4/virt-lib.m4 b/m4/virt-lib.m4
> new file mode 100644
> index 0000000..669ec66
> --- /dev/null
> +++ b/m4/virt-lib.m4
> @@ -0,0 +1,391 @@
> +dnl
> +dnl virt-lib.m4: Helper macros for checking for libraries

> +
> +dnl Probe for existance of libXXXX and set WITH_XXX

s/existance/existence/

> +dnl config header var, WITH_XXXX make conditional and
> +dnl with_XXX configure shell var.
> +dnl
> +dnl  LIBVIRT_CHECK_LIB([CHECK_NAME], [LIBRARY_NAME],
> +dnl                    [FUNCTION_NAME], [HEADER_NAME])
> +dnl

> +dnl
> +AC_DEFUN([LIBVIRT_CHECK_LIB],[
> +  m4_pushdef([check_name], [$1])
> +  m4_pushdef([library_name], [$2])
> +  m4_pushdef([function_name], [$3])
> +  m4_pushdef([header_name], [$4])
> +
> +  m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))

Pedantic: Underquoted (using a library named 'libdnl' would wreak havoc)

Real (but harmless) bug: you used the wrong quote characters ([] not
`'); not to mention autoconf already provides a helper macro m4_tolower
that does the same thing in less typing.  Recommendation: replace this
line with either:

libvirt-quality:
m4_pushdef([check_name_lc], m4_tolower(check_name))

pedantic-quality:
m4_pushdef([check_name_lc], m4_tolower(m4_defn([check_name])))

> +
> +  m4_pushdef([config_var], [WITH_]check_name)

Pedantic-quality:
m4_pushdef([config_var], [WITH_]m4_defn([check_name]))

> +  m4_pushdef([make_var], [WITH_]check_name)
> +  m4_pushdef([cflags_var], check_name[_CFLAGS])
> +  m4_pushdef([libs_var], check_name[_LIBS])
> +  m4_pushdef([arg_var], [with-]check_name_lc)
> +  m4_pushdef([with_var], [with_]check_name_lc)

etc. with s/var/m4_defn([var])/

> +
> +  AC_ARG_WITH(check_name_lc,
> +    [AS_HELP_STRING([--arg_var],
> +                    [with lib]]m4_dquote(library_name)[[ support @<:@default=check@:>@])],
> +    [],[with_var][=check])

We hashed this one out on IRC :)  It's fine for libvirt, but for
pedantic quality, it would be better as:

AC_ARG_WITH(m4_defn([check_name_lc]),
  [AS_HELP_STRING([--]m4_defn([arg_var]), [with lib]]m4_dquote(
       m4_defn([library_name]))[[ support @<:@default=check@:>@])],
  [], m4_defn([with_var])[=check])

> +
> +  old_LIBS="$LIBS"
> +  old_CFLAGS="$CFLAGS"

Pedantic: The "" are not necessary here, but don't hurt either.

> +  m4_expand(cflags_var[=])
> +  m4_expand(libs_var[=])

Overkill; you could get by with:

libvirt quality:
cflags_var=
libs_var=

Pedantic quality:
m4_defn([cflags_var])=
m4_defn([libs_var])=

> +
> +  fail=0
> +  if test "x$with_var" != "xno" ; then

Pedantic:
if test "x$m4_defn([with_var])" != "xno"; then

> +    if test "x$with_var" != "xyes" && test "x$with_var" != "xcheck" ; then
> +      m4_expand(cflags_var=["-I$with_var/include"])
> +      m4_expand(libs_var=["-L$with_var/lib"])

Overkill.
libvirt-quality:
  cflags_var="-I$with_var/include"

Pedantic:
m4_defn([cflags_var])="-I$m4_defn([with_var])/include"

> +    fi
> +    CFLAGS="$CFLAGS $cflags_var"
> +    LIBS="$LIBS $libs_var"
> +    AC_CHECK_LIB(library_name, function_name, [],[

AC_CHECK_LIB(m4_defn([library_name]), m4_defn([function_name]), [], [

> +      if test "x$with_var" != "xcheck"; then
> +        fail=1
> +      fi
> +      m4_expand(with_var[=no])

m4_defn([with_var])=no

> +    ])
> +    if test "$fail" = "0" && test "x$with_var" != "xno" ; then
> +      AC_CHECK_HEADER([header_name], [

AC_CHECK_HEADER(m4_defn([header_name]), [

> +        m4_expand(with_var[=yes])

m4_defn([with_var])=yes

> +      ],[
> +        if test "x$with_var" != "xcheck"; then
> +          fail=1
> +        fi
> +        m4_expand(with_var[=no])
> +      ])
> +    fi
> +  fi
> +
> +  LIBS="$old_LIBS"
> +  CFLAGS="$old_CFLAGS"

Again, "" aren't needed, but don't hurt.

> +
> +  if test $fail = 1; then
> +    AC_MSG_ERROR([You must install the lib]library_name[ library & headers to compile libvirt])

It would be really nice if you could run './configure' once and know
_all_ of the libraries to be installed, rather than having to run once
per library because each missing library aborts the script immediately.
 I can probably do that as a followup patch, where instead of directly
issuing the error, we instead append the latest error string to a series
of messages, then use a single m4_wrap to do AC_MSG_ERROR at the end of
all collected messages.  But that doesn't impact this patch.

> +  else
> +    if test "x$with_var" = "xyes" ; then
> +      if test "x$libs_var" = 'x' ; then
> +        m4_expand(libs_var=["-l]library_name["])

Overkill.

libvirt-quality:
  libs_var="-l[]library_name"

pedantic:
m4_defn([libs_var])="-l[]m4_defn([library_name])"

> +      else
> +        m4_expand(libs_var=["$libs_var -l]library_name["])
> +      fi
> +      AC_DEFINE_UNQUOTED(config_var, 1, [whether lib]library_name[ is available])

Pedantic:
AC_DEFINE_UNQUOTED(m4_defn([config_var]), [1],
  [whether lib]m4_defn([library_name])[ is available])

> +    fi
> +
> +    AM_CONDITIONAL(make_var, [test "x$with_var" = "xyes"])
> +
> +    AC_SUBST(cflags_var)

AC_SUBST(m4_defn([cflags_var]))

> +dnl
> +dnl     CHECK_NAME_ALT: Suffix/prefix used to set additional
> +dnl                     variables if alternative check suceeds

s/suceeds/succeeds/

> +dnl                      config.h: WITH_XXX macro
> +dnl                      Makefile: WITH_XXX conditional
> +dnl                    NB all vars for CHECK_NAME are also set
> +dnl   LIBRARY_NAME_ALT: alternative library name to check for
> +dnl  FUNCTION_NAME_ALT: alternative function name to check for
> +dnl    HEADER_NAME_ALT: alterantive header file to check for

s/alterantive/alternative/

> +  m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))

Again, wrong quote characters, and m4_tolower already does this for you.

> +
> +dnl
> +dnl Probe for existance of libXXXX and set WITH_XXX

s/existance/existence/

> +AC_DEFUN([LIBVIRT_CHECK_PKG],[
> +  m4_pushdef([check_name], [$1])
> +  m4_pushdef([pc_name], [$2])
> +  m4_pushdef([pc_version], [$3])
> +
> +  m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))

And again.


> +  AC_ARG_WITH(check_name_lc,
> +    [AS_HELP_STRING([--arg_var],
> +                   [with ]]m4_dquote(pc_name)[[ (>= ]]m4_dquote(pc_version)[[) support @<:@default=check@:>@])],

This is a long line; you can break anywhere after a ( with no impact, to
stay within 80 columns, such as
  ... m4-dquote(
     pc_version)...

> +
> +dnl
> +dnl To be used after a call to LIBVIRT_CHECK_LIB,
> +dnl LIBVIRT_CHECK_LIB_ALT or LIBVIRT_CHECK_PKG
> +dnl to print the result status
> +dnl
> +dnl  LIBVIRT_RESULT_LIB([CHECK_NAME])
> +dnl
> +dnl  CHECK_NAME: Suffix/prefix used for variables / flags, in uppercase.
> +dnl
> +dnl  LIBVIRT_RESULT_LIB([SELINUX])
> +dnl
> +AC_DEFUN([LIBVIRT_RESULT_LIB],[
> +  m4_pushdef([check_name], [$1])
> +
> +  m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))

One more place for m4_tolower

> +++ b/m4/virt-result.m4

> +
> +AC_DEFUN([LIBVIRT_RESULT], [

No doc comment beforehand?

Basically, all of my suggestions for using more m4_defn are for safety
against tricky library names, but don't really impact the common case
where a library name cannot be confused with an m4 macro.  And libvirt
doesn't use any libraries with an unsafe name.

My priority listing of which things to fix (or ignore):

* spelling errors - must fix before committing
* use m4_tolower instead of m4_translit - please fix before committing
* doc comment for LIBVIRT_RESULT - please fix before committing
* avoid m4_expand - up to you, but it looks simpler if you fix to at
least libvirt quality
* use m4_defn in more places - up to you, and I'll look the other way if
you choose not to fix

ACK once you cover at least the first bullet, and preferably at least
the first 3 bullets; and of course if your testing of later patches in
the series shows that things still work.  I don't need to see a v2
unless you go for all 5 bullets.

(Part of me wonders if I should take the best parts of these macros,
make them more robust to quoting, and make it officially part of some
future release of autoconf.)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]