Re: [PATCH v2 1/2] crypto, sha1: export sha1_update for reuse

[Herbert Xu]

On Thu, Jul 28, 2011 at 05:29:35PM +0200, Mathias Krause wrote:
> On Thu, Jul 28, 2011 at 4:58 PM, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Sun, Jul 24, 2011 at 07:53:13PM +0200, Mathias Krause wrote:
> >>
> >> diff --git a/include/crypto/sha.h b/include/crypto/sha.h
> >> index 069e85b..7c46d0c 100644
> >> --- a/include/crypto/sha.h
> >> +++ b/include/crypto/sha.h
> >> @@ -82,4 +82,9 @@ struct sha512_state {
> >> Â Â Â u8 buf[SHA512_BLOCK_SIZE];
> >> Â};
> >>
> >> +#if defined(CONFIG_CRYPTO_SHA1) || defined (CONFIG_CRYPTO_SHA1_MODULE)
> >> +extern int crypto_sha1_update(struct shash_desc *desc, const u8 *data,
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned int len);
> >> +#endif
> >
> > Please remove the unnecessary #if.
> 
> The function will only be available when crypto/sha1_generic.o will
> either be build into the kernel or build as a module. Without the #if
> a potential wrong user of this function might not be detected as early
> as at compilation time but as late as at link time, or even worse, as
> late as on module load time -- which is pretty bad. IMHO it's better
> to detect errors early, as in reading "error: implicit declaration of
> function âcrypto_sha1_updateâ" when trying to compile the code in
> question instead of "Unknown symbol crypto_sha1_update" in dmesg when
> trying to load the module. That for I would like to keep the #if.

In order to be consistent please remove the ifdef.  In most
similar cases in the crypto subsystem we don't do this.  As
adding such ifdefs all over the place would gain very little,
I'd much rather you left it out.

The one case where this would make sense is if it were a trivial
inline in the !defined case.

Thanks!
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/