Klaus Heinrich Kiwi wrote: > 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) > I was actually asking about what it implemented and was removed, not defending the _code_ itself, I might say additionally that I agree with the uniform layout it (the code) is getting. My point is: if configure script detects that libica is installed on the machine, the ica token compilation must be enabled automatically. It would save us support time. Also, the architecture handling isn't a big deal indeed, the libica installation would already handled that. > >>> +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 > But, if the xcryptolinz was detected in the search path, or in the provided library path, why not enable automatically the pkcscca_migrate tool compilation too? pkcscca_migrate implements the token data migration from an old Master Key to a new one, since the Master Key renewal should be made quite often, I don't think this tool is optional in case the CCA token is going to be compiled/used. Note that even making it automatically, the user will still be left with the freedom to disable it. > 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. > My above response on the architecture handling fits here well too, just enable the icatoken in case libica was detected. >>> 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. > Thanks for explaining it, agreed. That's worth putting in the commit log though. > 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?). > (Yes) >>> 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, I've already got the context, splitting it now wouldn't help that much, at least for me.
Rajiv ------------------------------------------------------------------------------ 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
