[V2 PATCH] Implement SM3 hash algorithm in gnulib and the computing tool in coreutils.

[Jia Zhang]

Hi Pádraig & Bruno,

This is the V2 review request for SM3 hash algorithm.

As promised, I have made SM3 hash algorithm available in the latest
libgcrypt. See
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=4423bf3cc4432b9bfe801ff74cb05e6f0dd3eccd

The V2 patch adds the support with libgcrypt and associated test case.
By the way, I fixed several gc-libgcrypt and gc-gnulib build failures in
order to build out sm3sum with either libgcrypt or internal functions.

Plz refer to the following PRs for code review.

- For the new gnulib module crypto/sm3
https://github.com/coreutils/gnulib/pull/3/commits

- For the new program sm3sum
https://github.com/coreutils/coreutils/pull/12/commits

Thanks,
Jia

[Bruno Haible]

[Dropping coreutils, since I'm replying only the gnulib part.]

Hi Jia,

Post by Jia Zhang
Plz refer to the following PRs for code review.
- For the new gnulib module crypto/sm3
https://github.com/coreutils/gnulib/pull/3/commits

Usually, on this mailing list, we share patches through 'git format-patch -1',
not github. In order to view your patch, I had to
1. go to https://github.com/jiazhang0/gnulib/tree/sm3-devel
2. do a 'git clone' with the URL from there,
3. $ git checkout origin/sm3-devel
4. $ git format-patch -5

Review comments:
* "New module: crypto/sm3" looks very good. Just one thing is wrong:
The HAVE_OPENSSL_SM3 check is not like in, say, m4/sha1.m4.

You can see the problem by doing
$ ./gnulib-tool --create-testdir --dir=testdir1 --single-configure crypto/sm3
$ cd testdir1
$ ./configure CPPFLAGS=-Wall
$ make
...
gcc -g -O2 -o test-sm3 test-sm3.o libtests.a ../gllib/libgnu.a libtests.a @LIB_CRYPTO@
gcc: error: @LIB_CRYPTO@: No such file or directory
Makefile:919: recipe for target 'test-sm3' failed

How to fix this?
If sm3 is already supported in openssl: use gl_CRYPTO_CHECK, like in sha1.m4.
If not, initialize LIB_CRYPTO to empty, in a way that does not conflict with
the other crypto modules:
m4_divert_once([DEFAULTS], [LIB_CRYPTO=])
AC_SUBST([LIB_CRYPTO])

* Patch 2 looks good, applied.

* Patch 3 looks good, applied.

* Patch 4, 5: GCC's warnings were correct (unlike GCC's warning for 'proper_name').
Instead of disabling the warning, could you mark the functions with
_GL_ATTRIBUTE_CONST?

Bruno

[Jia Zhang]

Post by Bruno Haible
[Dropping coreutils, since I'm replying only the gnulib part.]
Hi Jia,

Post by Jia Zhang
Plz refer to the following PRs for code review.
- For the new gnulib module crypto/sm3
https://github.com/coreutils/gnulib/pull/3/commits

Usually, on this mailing list, we share patches through 'git
format-patch -1', not github. In order to view your patch, I had to
1. go to https://github.com/jiazhang0/gnulib/tree/sm3-devel 2. do a
'git clone' with the URL from there, 3. $ git checkout
origin/sm3-devel 4. $ git format-patch -5

Sorry I will send V3 patch to here directly.

Post by Bruno Haible
Review comments: * "New module: crypto/sm3" looks very good. Just
one thing is wrong: The HAVE_OPENSSL_SM3 check is not like in,
say, m4/sha1.m4.
You can see the problem by doing $ ./gnulib-tool --create-testdir
--dir=testdir1 --single-configure crypto/sm3 $ cd testdir1 $
./configure CPPFLAGS=-Wall $ make ... gcc -g -O2 -o test-sm3
recipe for target 'test-sm3' failed
How to fix this? If sm3 is already supported in openssl: use
gl_CRYPTO_CHECK, like in sha1.m4. If not, initialize LIB_CRYPTO to
empty, in a way that does not conflict with the other crypto
modules: m4_divert_once([DEFAULTS], [LIB_CRYPTO=])
AC_SUBST([LIB_CRYPTO])

Thanks for your comments. I fixed it and the test passes. I just feel
strange that test-sm3 was built out without error if I compiled gnulib
with coreutils together.

Post by Bruno Haible
* Patch 2 looks good, applied.
* Patch 3 looks good, applied.
* Patch 4, 5: GCC's warnings were correct (unlike GCC's warning
for 'proper_name'). Instead of disabling the warning, could you
mark the functions with _GL_ATTRIBUTE_CONST?

No problem. I will correct them.

Thanks,
Jia

Post by Bruno Haible
Bruno

[Bruno Haible]

Post by Jia Zhang
Thanks for your comments. I fixed it and the test passes. I just feel
strange that test-sm3 was built out without error if I compiled gnulib
with coreutils together.

It passed with coreutils because coreutils requests other crypto modules
as well, and these set LIB_CRYPTO.

The particular thing about Gnulib is that people can use an *arbitrary
subset* of its set of modules. Therefore, when we add a module we must
make sure that
1. the module works standalone, without any other modules,
2. conflicts with other modules are avoided.
'gnulib-tool --create-testdir' is the tool for verifying this.

Bruno

[Jia Zhang]

Post by Bruno Haible

Post by Jia Zhang
Thanks for your comments. I fixed it and the test passes. I just
feel strange that test-sm3 was built out without error if I
compiled gnulib with coreutils together.

It passed with coreutils because coreutils requests other crypto
modules as well, and these set LIB_CRYPTO.
The particular thing about Gnulib is that people can use an
*arbitrary subset* of its set of modules. Therefore, when we add a
module we must make sure that 1. the module works standalone,
without any other modules, 2. conflicts with other modules are
avoided. 'gnulib-tool --create-testdir' is the tool for verifying
this.

Make sense. Thanks you very much!

Jia

Post by Bruno Haible
Bruno