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

[Jia Zhang]

于 2017/10/28 下午8:44, Bruno Haible 写道:
> [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

Sorry I will send V3 patch to here directly.

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

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.

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

> 
> Bruno
>