On 09/29/2009 11:01 AM, Rajiv Andrade wrote: > Hello Klaus, comments below > Klaus Heinrich Kiwi wrote: >> -# >> -# s390 tokens >> -# >> - >> -# The ica token is enabled by default on s390 >> -if [[ $s390 -eq 1 ]]; then >> - >> > I see this check removed and and can't see $s390 being read anywhere > else, am I wrong to say that now the ICA token isn't being built by > default only on s390 anymore?
The AC_ARG_ENABLE() macro conditioned to if [[ s390 ]] feels pretty nonsensical to me. So I'm defining all AC_ARG_ENABLE() macros the same way for all architectures, and using <root>/usr/lib/pkcs11/Makefile.am choose between building ica_stdll and ica_s390_stdll (btw, it's also my intention to remove ica_stdll and move ica_s390_stll in it's place in the future, since I don't see a reason for two versions) >> +dnl --- with_xcryptolinz >> +XCRYPTOLINZ_CFLAGS= >> +XCRYPTOLINZ_LIBS= >> +if test "x$with_xcryptolinz" != "xno"; then >> + if test "x$with_xcryptolinz" != "xyes" -a "x$with_xcryptolinz" != >> "xcheck"; then >> + XCRYPTOLINZ_CFLAGS="-I$with_xcryptolinz" >> + XCRYPTOLINZ_LIBS="-L$with_xcryptolinz" >> + fi >> + old_cflags="$CFLAGS" >> + old_libs="$LIBS" >> + CFLAGS="$CFLAGS $XCRYPTOLINZ_CFLAGS" >> + LIBS="$LIBS $XCRYPTOLINZ_LIBS" >> +dnl - The above may not be necessary since opencryptoki brings this header >> file anyway. >> + AC_CHECK_HEADER([csulincl.h], [], [ >> + if test "x$with_xcryptolinz" != "xcheck"; then >> + AC_MSG_ERROR([Build with xcryptolinz requested but >> xcryptolinz headers couldn't be found]) >> + fi >> + with_xcryptolinz=no >> + ]) >> + if test "x$with_xcryptolinz" != "xno"; then >> + AC_CHECK_LIB([csulsapi], [CSNBKTC], >> + [with_xcryptolinz=yes], [ >> > If xcryptolinz lib was found, we can conclude that the user will > probably want pkcscca_migrate, and also the CCA token to be built, what > about enforcing it here? Let me clarify my intentions here. I'm using AC_ARG_ENABLE() macro only for enabling/disabling building portions of the code, while I'm using AC_ARG_WITH() only to specify additional libraries that may or may not be needed by any portion of the code. All this in an architecture-independent way, so that there's no confusion about what is enabled and what not. For all AC_ARG_ENABLE() macros, I give the user the default setting on the help string. AC_ARG_WITH() macros all default to 'check', that is. the configure script will try to find them in the search path. The user can also specify where to find them with --with-package=DIR though. The above means that if the xcryptolinz library is in the search path, the user only need to specify --enable-pkcscca_migrate to build this tool, otherwise, he will need to specify the location with --with-xcryptolinz=/path/to/lib It's also worth noting that certain archs *will* need to explicitly disable some features in order to build, i.e. the ica token. That's simply because libica is not available for archs other than s390[x]. And that was exactly my intention in making the build defaults not dependent of the underlying platform. >> CFLAGS="$CFLAGS -DPKCS64 \ >> @@ -300,9 +613,7 @@ AC_OUTPUT([Makefile usr/Makefile \ >> usr/sbin/Makefile \ >> usr/sbin/pkcsslotd/Makefile \ >> usr/sbin/pkcs11_startup/Makefile \ >> - usr/sbin/pkcs11_startup/pkcs11_startup \ >> usr/sbin/pkcs_slot/Makefile \ >> - usr/sbin/pkcs_slot/pkcs_slot \ >> usr/sbin/pkcsconf/Makefile \ >> usr/sbin/pkcscca_migrate/Makefile \ >> usr/lib/pkcs11/methods/Makefile \ >> @@ -321,7 +632,6 @@ AC_OUTPUT([Makefile usr/Makefile \ >> usr/lib/pkcs11/cca_stdll/tok_struct.h \ >> usr/lib/pkcs11/methods/4758_status/Makefile \ >> misc/Makefile \ >> - misc/pkcsslotd \ >> > I probably need to read the other patches to understand why these was > removed. Following recommendation from the Autoconf/Automake documentation, I'm not processing scripts using autoconf anymore. Instead, I'm using a sed oneliner in each script's Makefile.am. This allow us to process pathnames macros only at build time instead of configure time. Note that I'm also not relying on AC_SUBST() macros and [SBIN|CONFIG|DB]_PATH anymore. In summary, this allow us to be more flexible when building/installing at non-standard paths (did you know that the current code only allow us to install to /usr/lib or /usr/local/lib?). >> testcases/Makefile \ >> testcases/common/Makefile \ >> testcases/driver/Makefile \ >> > That's it, by the way, nice work refactoring/cleaning this up! > No problem. I have some corrections and additions on top of this first batch. But this means even more things to review. I wish I could split those in more digestible pieces, but that would also mean some additional work for my side. I'll send the second revision for this batch and if it's too much for one shot we can discuss splitting them. Thanks for the review, -Klaus -- Klaus Heinrich Kiwi | [email protected] | http://blog.klauskiwi.com Open Source Security blog : http://www.ratliff.net/blog IBM Linux Technology Center : http://www.ibm.com/linux/ltc ------------------------------------------------------------------------------ Come build with us! The BlackBerry® Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9-12, 2009. Register now! http://p.sf.net/sfu/devconf _______________________________________________ Opencryptoki-tech mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
