A little cleanup for SVN 1283

[Dirk Stoecker]

Hello,

>> 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.

No. Why do you want care for any other macro?

HAVE_CONFIG_H only means, that a file config.h can be included or not. In 
case HAVE_CONFIG_H is not set, the configuration must be supplied using 
other means (e.g. defines in project configuration settings).

Not using HAVE_CONFIG_H prevents these other means and restricts the whole 
system to a config.h. Actually this is exactly what I do under Windows. I 
have the definitions in the project configuration.

> 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.

All this goes together with the Makefile.am changes.
The "-I$(top_srcdir)/mpi" allows including mpi-files. This goes together 
with the removal of "../mpi/" below.
The part ../src is obsolete. This is and must be equal to "$(top_srcdir)", 
as ".." is what $(top_srcdir) is. And this is included.

>  --- 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.

This is a normal C forward declaration, which is perfectly valid. Don't 
know the reason, where I required this. Probably Windows again. Maybe the 
reason is gone already.

>  --- 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.

It was. This is a legacy test. gcrypt.h and g10lib.h have no longer 
conflicts.

>  --- 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.

See above. No absolute path, but instead of this proper include 
definitions. This ../mpi/ is an ugly fix for the fact, that mpi was not 
included in the include parts of Makefile.am.

>  --- 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.

Again the same. All of the makefile.am changes cleanup the build to use 
proper definitions. There is no longer any need for "../" paths, which 
makes different building easier.

>  --- 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.

It is wrong. You don't release the memory. Usually this will do no harm, 
but a test must be correct. Errors in test must be fixed as any normal 
errors. Especially as the tests are very likely used as user 
implementation examples.

You will be happy about it, whenever memory tracking tests will be added 
(which is what I did with the tests).

>  --- 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:

Well, same as above. Freeing memory only.

>  --- 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

Then the problem lies elsewhere. Without this the docs do not build from 
scratch.

>  -#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.

That is not true. The file config.h defines the values to 0 when disabled.
Maybe that behaviour changed during autoconf/automake development, but now
USE_ELGAMAL is always defined.

>  --- 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?

sys.un.h is not required. gcrypt.h does WIN32 handling for <sys/socket.h>.
Optional would be:
#ifdef _WIN32
#include <windows.h>
#else
#include <sys/types.h>
#include <sys/socket.h>
#endif

>         /* 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.

I fixed bugs I found to get stuff working. I didn't care if these is 
legacy or not. Bug is bug. If it is removed in the future, who cares, but 
until then it is fixed.

Ciao
-- 
http://www.dstoecker.eu/ (PGP key available)