Date: Wed, 25 Jul 2012 03:57:02 -0400 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Subject: Re: Re: crypt* files in crypt directory 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.