On 01/10/2013 01:18 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list