[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [V2 PATCH] Implement SM3 hash algorithm in gnulib and the computing
From: |
Bruno Haible |
Subject: |
Re: [V2 PATCH] Implement SM3 hash algorithm in gnulib and the computing tool in coreutils. |
Date: |
Sat, 28 Oct 2017 14:44:16 +0200 |
User-agent: |
KMail/5.1.3 (Linux/4.4.0-97-generic; KDE/5.18.0; x86_64; ; ) |
[Dropping coreutils, since I'm replying only the gnulib part.]
Hi Jia,
> 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