Re: Re: crypt* files in crypt directory

[Rich Felker <dalias@aerifal.cx>]

Hi. Finally got around to reviewing this a bit.. :-)

On Sat, Jul 21, 2012 at 05:23:24PM +0200, Łukasz Sowa wrote:
> --- /dev/null
> +++ b/src/misc/crypt_blowfish.c
> [...]
> +#include <errno.h>
> +#ifndef __set_errno
> +#define __set_errno(val) errno = (val)
> +#endif

Is there a reason for this __set_errno stuff? IMO the code should just
directly assign to errno. This looks like some silly cargo-culting
from glibc...

> +/* Just to make sure the prototypes match the actual definitions */
> +#include <crypt.h>

I'm not sure this is useful; this file does not define any public
interfaces.

> +#ifdef __i386__
> +#define BF_ASM				1
> +#define BF_SCALE			1
> +#elif defined(__x86_64__) || defined(__alpha__) || defined(__hppa__)
> +#define BF_ASM				0
> +#define BF_SCALE			1
> +#else
> +#define BF_ASM				0
> +#define BF_SCALE			0
> +#endif

Is this used/needed for anything? I'm generally opposed to having
different versions of code conditionally compiled for different archs
unless there's a very good reason. It makes it so testing on multiple
archs is required to ensure that there are not bugs.

> +typedef unsigned int BF_word;
> +typedef signed int BF_word_signed;

While it's okay for musl where all targets have 32-bit int, if you
want a type that's 32-bit you should probably be using [u]int32_t, or
if you want the system wordsize then int is wrong and you should be
using long or size_t or something...

> +#if BF_ASM
> +#define BF_body() \
> +	_BF_body_r(&data.ctx);
> +#else

This does not seem to exist in the submitted code..

> +char *_crypt_gensalt_blowfish_rn(const char *prefix, unsigned long count,
> +	const char *input, int size, char *output, int output_size)

This belongs in the gensalt file, not here. There's no reason every
program that merely wants to authenticate against passwords should
pull in salt-generation code.

Aside from that, the code looks a bit long as-is, but I haven't tried
building it yet and it might just look that way from all the comments.
I'll take a look at size stuff soon..

Rich