On 2022-01-04 at 21:01:19, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
> > +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
> > +endif
> > +
> > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
> > +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
> > +	EXTLIBS += -lbsd
> > +endif
> > +
> > +ifeq ($(strip $(CSPRNG_METHOD)),getrandom)
> > +	BASIC_CFLAGS += -DHAVE_GETRANDOM
> > +endif
> > +
> > +ifeq ($(strip $(CSPRNG_METHOD)),getentropy)
> > +	BASIC_CFLAGS += -DHAVE_GETENTROPY
> > +endif
> > +
> > +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom)
> > +	BASIC_CFLAGS += -DHAVE_RTLGENRANDOM
> > +endif
> > +
> > +ifeq ($(strip $(CSPRNG_METHOD)),openssl)
> > +	BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG
> > +endif
> 
> Use of $(strip ($VAR)) looks a bit different from what everybody
> else does with ifeq in this Makefile.  Was there a particular reason
> to use it that I am missing?

As far as I'm aware, ifeq includes whitespace in its comparisons (at
least, I was led to believe that by the documentation for GNU make).
However, in many places, we insert leading spaces before the variable
name.  Some testing shows that GNU make strips those off, although my
earlier testing led me to believe otherwise.

So I'll change this in v3.

> When we see something unrecognized in CSPRNG_METHOD, we do not touch
> BASIC_CFLAGS (or EXTLIBS) here.  I wonder how easy a clue we would
> have to decide that we need to fall back to urandom.  IOW, I would
> have expected a if/else if/... cascade that has "no we do not have
> anything else and need to fall back to urandom" at the end.

I tried to avoid the if/else if cascade for the same reasons that we do
in config.mak.uname, which is that it gets pretty hard to reason about
after you have more than just a few items.

> > diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
> > new file mode 100644
> > index 0000000000..65d14973c5
> > --- /dev/null
> > +++ b/t/helper/test-csprng.c
> > @@ -0,0 +1,29 @@
> > +#include "test-tool.h"
> > +#include "git-compat-util.h"
> > +
> > +
> > +int cmd__csprng(int argc, const char **argv)
> > +{
> > +	unsigned long count;
> > +	unsigned char buf[1024];
> > +
> > +	if (argc > 2) {
> > +		fprintf(stderr, "usage: %s [<size>]\n", argv[0]);
> > +		return 2;
> > +	}
> > +
> > +	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
> > +
> > +	while (count) {
> > +		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
> 
> "chunk" should be of type "size_t", no?

Yes, it should.  Will fix in v3.

> > diff --git a/wrapper.c b/wrapper.c
> > index 36e12119d7..1052356703 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags)
> >  	return open(path, flags);
> >  #endif
> >  }
> > +
> > +int csprng_bytes(void *buf, size_t len)
> > +{
> > +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)
> 
> Shouldn't HAVE_ARC4RANDOM mean "we have arc4random_buf() function
> available; please use that.", i.e. wouldn't this give us cleaner
> result in the change to the Makefile?
> 
>  ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
>  	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
>  endif
>  
>  ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
> -	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
> +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
>  	EXTLIBS += -lbsd
>  endif
> 
> To put it differently, C source, via BASIC_CFLAGS, would not have to
> care where the function definition comes from.  It is linker's job
> to care and we are already telling it via EXTLIBS, so I am not sure
> the value of having HAVE_ARC4RANDOM_LIBBSD as a separate symbol.

There's an important additional difference here.  On real BSD systems,
the prototypes for this family of functions live in <stdlib.h>.
Obviously, on Linux with libbsd, that's not the case.  So, in
git-compat-util.h, we need to include <bsd/stdlib.h> to get the
prototype, and that's done only if HAVE_ARC4RANDOM_LIBBSD is set, since
on a real BSD system, that header isn't present.

If you'd prefer, I can set both flags here, one which controls the
function use and one which controls the #define, or I can leave it as it
is.

> OK, I earlier worried about the lack of explicit "we are using
> urandom" at the Makefile level, but as long as this will remain the
> only place that needs to care about the fallback, the above is
> perfectly fine.

Yes, I tried very hard to make this the only place the default mattered.

And, for the record, I think /dev/urandom is the sensible default here,
because I know it exists on some of the proprietary Unix systems, like
Solaris, where I believe it originated, and QNX, as well as Linux and
the BSDs.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA