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: 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. s/existance/existence/ 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]))) Pedantic-quality: m4_pushdef([config_var], [WITH_]m4_defn([check_name])) etc. with s/var/m4_defn([var])/ 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]) Pedantic: The "" are not necessary here, but don't hurt either. Overkill; you could get by with: libvirt quality: cflags_var= libs_var= Pedantic quality: m4_defn([cflags_var])= m4_defn([libs_var])= Pedantic: if test "x$m4_defn([with_var])" != "xno"; then Overkill. libvirt-quality: cflags_var="-I$with_var/include" Pedantic: m4_defn([cflags_var])="-I$m4_defn([with_var])/include" AC_CHECK_LIB(m4_defn([library_name]), m4_defn([function_name]), [], [ m4_defn([with_var])=no AC_CHECK_HEADER(m4_defn([header_name]), [ m4_defn([with_var])=yes Again, "" aren't needed, but don't hurt. 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. Overkill. libvirt-quality: libs_var="-l[]library_name" pedantic: m4_defn([libs_var])="-l[]m4_defn([library_name])" Pedantic: AC_DEFINE_UNQUOTED(m4_defn([config_var]), [1], [whether lib]m4_defn([library_name])[ is available]) AC_SUBST(m4_defn([cflags_var])) s/suceeds/succeeds/ s/alterantive/alternative/ Again, wrong quote characters, and m4_tolower already does this for you. s/existance/existence/ > +AC_DEFUN([LIBVIRT_CHECK_PKG],[ > + m4_pushdef([check_name], [$1]) > + m4_pushdef([pc_name], [$2]) > + m4_pushdef([pc_version], [$3]) > + And again. 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]) > + One more place for m4_tolower 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