A little cleanup for SVN 1283

[Werner Koch]

On Wed, 19 Mar 2008 12:58, gcrypt at dstoecker.de said:

> The changes are pretty self-explaining and straightforward.

Unfortunately this is not the case; please explain the the changes.

> I hereby declare that this patch is Public Domain and may be used and
> relicensed by everybody in any form.

For a GNU project this is not sufficient; we need to do do some paper
work.

> P.S. Please surround all the "#include <config.h>" by
> "#ifdef HAVE_CONFIG_H". This does no harm for autoconf version, but
> allows to use the files outside autoconf environment.

That is not a good idea.  To do this we would need to figure out what
macros are used by that file and clearly docuemntthis.  Without that the
code may or may not work.  In most cases config.h is an integral part of
the file.


Some notes or questions:

  --- mpi/Makefile.am	(Revision 1283)
  +++ mpi/Makefile.am	(Arbeitskopie)
  @@ -21,9 +21,7 @@
   # I was not able to build it with 64Megs - 1.6 fixes this.
   # not anymore required: AUTOMAKE_OPTIONS = 1.6
   
  -# Need to include ../src in addition to top_srcdir because gcrypt.h is
  -# a built header.
  -AM_CPPFLAGS = -I../src -I$(top_srcdir)/src
  +AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/mpi
   AM_CFLAGS = $(GPG_ERROR_CFLAGS)

As stated in the comment ../src is required.  Think of VPATH builds.

  --- src/gcrypt.h.in	(Revision 1283)
  +++ src/gcrypt.h.in	(Arbeitskopie)
  @@ -41,6 +41,8 @@
   
   #include <sys/time.h>
   
  +struct msghdr;

You can't do that.  If your system does not provide the declaration in a
system header as included by gcrypt.h, the correct solution is to do a
configure tests ala FALLBACK_SOCKLEN_T.

  --- src/g10lib.h	(Revision 1283)
  +++ src/g10lib.h	(Arbeitskopie)
  @@ -25,10 +25,6 @@
   #ifndef G10LIB_H
   #define G10LIB_H 1
   
  -#ifdef _GCRYPT_H
  -#error  gcrypt.h already included
  -#endif

Why?  This is on purpose.

  --- src/mpi.h	(Revision 1283)
  +++ src/mpi.h	(Arbeitskopie)
  @@ -28,11 +28,11 @@
   #ifndef G10_MPI_H
   #define G10_MPI_H
   
   #include <config.h>
   #include <stdio.h>
   #include "types.h"
   #include "memory.h"
  -#include "../mpi/mpi-asm-defs.h"
  +#include "mpi-asm-defs.h"

I can't see a reasosn for this change.
   
  --- src/Makefile.am	(Revision 1283)
  +++ src/Makefile.am	(Arbeitskopie)
  @@ -20,6 +20,7 @@
   
   ## Process this file with automake to produce Makefile.in
   
  +INCLUDES = -I$(top_srcdir)/mpi

Please explain the problem.

  --- tests/pubkey.c	(Revision 1283)
  +++ tests/pubkey.c	(Arbeitskopie)
  @@ -125,6 +125,7 @@
     /* Extract data from plaintext.  */
     l = gcry_sexp_find_token (plain0, "value", 0);
     x0 = gcry_sexp_nth_mpi (l, 1, GCRYMPI_FMT_USG);
  +  gcry_sexp_release (l);

Well, yes we could do that.  Howerver this regression test is not here
to tests allocatins ;-).  For the other test it is a bit too hard to see
from the diffs what you are going to change.  Anyway, these are
regression tests and thus we can't change an existing test because that
one tests existing behaviour.  Always add new tests if you want to test
more.

  --- tests/ac-schemes.c (Revision 1283)
  +++ tests/ac-schemes.c	(Arbeitskopie)
  @@ -320,6 +320,14 @@
         es_checks (handle, key_public, key_secret);
         ssa_checks (handle, key_public, key_secret);
       }
  +
  +  if(handle)
  +    gcry_ac_close (handle);
  +
  +  if(key_secret)
  +    gcry_ac_key_destroy (key_secret);
  +  if(key_public)
  +    gcry_ac_key_destroy (key_public);
 
Alright, you figured one spot.  However there is bunch of more problems
in the ac fucntions and worst of all no clear concept of ownership.  I
once started to fix that but gave up and it is likely that the ac stuff
will eventually be removed.  See the manual:

  @strong{This interface has a few known problems; most noteworthy an
  inherent tendency to leak memory.  It might not be available in
  forthcoming versions Libgcrypt.}

  --- doc/Makefile.am	(Revision 1283)
  +++ doc/Makefile.am	(Arbeitskopie)
  @@ -33,3 +33,7 @@
   	  $${user}@cvs.gnupg.org:$${dir} ); \
           rsync -v gcrypt.pdf gcrypt.info $${user}@cvs.gnupg.org:$${dir}
   
  +gcrypt.info: version.texi
  +
  +version.texi: stamp-vti
  +

Automake already handles this.  No need to add it to Makefile.am

  -#ifdef USE_ELGAMAL
  +#if USE_ELGAMAL

Why? It is either defined or not.  Autoconf also make sure that it is
never defined to 0 thus it is only a matter of style what to use.

  --- cipher/cipher.c	(Revision 1283)
  +++ cipher/cipher.c	(Arbeitskopie)
  @@ -747,6 +749,7 @@
   	  h->mode = mode;
   	  h->flags = flags;
   
  +#if USE_AES
             /* Setup bulk encryption routines.  */
             switch (algo)
               {
  @@ -762,6 +765,7 @@
               default:
                 break;
               }
  +#endif /* USE_AES */

Good catch.

  --- cipher/rndegd.c	(Revision 1283)
  +++ cipher/rndegd.c	(Arbeitskopie)
  @@ -27,8 +29,8 @@
   #include <string.h>
   #include <unistd.h>
   #include <sys/types.h>
  -#include <sys/socket.h>
  -#include <sys/un.h>
  +/*#include <sys/un.h>*/
  +#include "gcrypt.h"

Why?


         /* Store named MPI in data_set_new.  */
  -      err = gcry_ac_data_set (data_set_new, GCRY_AC_FLAG_DEALLOC, string, mpi);
  +      err = gcry_ac_data_set (data_set_new, GCRY_AC_FLAG_COPY |
  +                              GCRY_AC_FLAG_DEALLOC, string, mpi);

I appreciate the work you did to fix the ac problems.  However I doubt
that it will eventually succeeed. See above.




Salam-Shalom,

   Werner
   
  


-- 
Die Gedanken sind frei.  Auschnahme regelt ein Bundeschgesetz.