Mailing List Archive

On Wed, 19 Mar 2008 12:58, gcrypt@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.


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

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)

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Wed, 19 Mar 2008 16:05, gcrypt@dstoecker.de said:

> HAVE_CONFIG_H only means, that a file config.h can be included or

Right. But you don't have a choice in libgcrypt. There we demand that
the config.h as created by configure needs to there. It is a part of
the code. If you don't want that, you are on your own and then, if you
like, add @ifdef HAVE_CONFIG_H.

> whole system to a config.h. Actually this is exactly what I do under
> Windows. I have the definitions in the project configuration.

I am sorry, for you, but we use a different build system and as I have
said hundreds of times, building on Windows is not supported. A sed or awk
script to add the #ifdef HAVE_CONFIG_H si straightforward to implement
but in that case it would be easier to provide a condig.h (even if empty).

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

You are wrong. The ../src is required for VPATH builds. VPATH builds
are a Good Thing and actually required by the GNU standards and due to
"make distcheck".

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

It would clash with systems redefining msghdr using cpp tricks.

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

The semantics are different with your change.

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

Well, I want to use them. The directories are closely coupled and thus
it is better to express this with the file names.

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

This is a regression tests. By releasing the memory you change the test
and may not detect a regression. Okay, this is a bit of a theoretical
argument but I currently don't have the resources to fix and test the
regression tests.

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

These will be new tests.

> Well, same as above. Freeing memory only.

As told, the ac funmctions will go anyway. They are seriously bugged.

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

No problem here to bootstrap it.

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

You are right. The configure tests are broken. Probably we never build
without some algorithms for a long time. I'll fix configure.

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

POSIX requires sys/un.h as it defines struct sockaddr_un. Windows is
irrelevant here. If you want to run change the build system for
Windows, I suggest to provide an sys/un.h stub.

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

First of all I am not sure whether this is the correct fix. We have
identified several inconsistencies in ac module and found no way to
properly solve that without an API change.

Second, yes bugs are to be fixed. But sometimes it is better not to fix
a bug if it would introduce a couple of new and unknown bugs. A
maintainer's job is to overview the overall stability of a software and
not to fix bugs just because there is a bug. In particular not if it is
in a part scheduled for removal.


Shalom-Salam,

Werner



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


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

At Wed, 19 Mar 2008 16:05:34 +0100 (CET),
Dirk Stoecker <gcrypt@dstoecker.de> wrote:
> > --- 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.

Maybe you miss ./configure --enable-maintainer-mode

Thanks,
Marcus




_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel