From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 24456 invoked from network); 24 Nov 2022 02:59:16 -0000 Received: from zero.zsh.org (2a02:898:31:0:48:4558:7a:7368) by inbox.vuxu.org with ESMTPUTF8; 24 Nov 2022 02:59:16 -0000 ARC-Seal: i=1; cv=none; a=rsa-sha256; d=zsh.org; s=rsa-20210803; t=1669258756; b=c215wEH5ubE/XfTZrMpzYHFBANepipHHzCQCqiy0s3TVKmP22QBESXmkJHHaFYC/9OCsVGaJ1n +UsBMs3p8NegmGz4LgYVtaJzQQvjRJ/Tpz25yFuKDebTZUAPP6PonXsD+C9P1aHBA2orj/31v1 Q7b/NvWUF1QtZjVFdDTvX5NvCHd9pWwZufUE3C/K6IGFCD0XUFbjlP1SlKoG0o8ygUBr++Q3DR j9COoizLHPJcxfKFZd68fIJ8+BFKzr20Sm8OV17RMrux0Ko70uoNEq52syoGPIx0n67eZpZx9L K7aRWCk2t/PQGUU+fcodB7BVeMTXXM7dswVnsKYAbEvI+A==; ARC-Authentication-Results: i=1; zsh.org; iprev=pass (iris.zentaur.org) smtp.remote-ip=198.58.127.206; dkim=pass header.d=zentaur.org header.s=dkim20200120 header.a=rsa-sha256; dmarc=pass header.from=zentaur.org; arc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed; d=zsh.org; s=rsa-20210803; t=1669258756; bh=BeQE297AsCcrlr52qioCiO2/KAkjRj1Gsa5GEpYCxu4=; h=List-Archive:List-Owner:List-Post:List-Unsubscribe:List-Subscribe:List-Help: List-Id:Sender:Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:To:Subject:MIME-Version:Date:Message-ID:DKIM-Signature: DKIM-Signature; b=M4LwcTKo5CH1u863L9BwvjZldfesxOrKRZ899gk5Vip+5bzZGEXEKJi+7FbMW+Wg34HlELZMl2 dAPR8Wn66X6GoMYPJwalaNEbp57/QDhqBy6hc7M4zrgIauzkGr8kpr5oECwbz1xwXMehkShJY1 XuQ1iIhkWvrFhhSXTOYymOskpvVcFxCuxoLc8lVLlkKpaQ0j1PvjRiTvizY48P0ZWOcBuszJOx WiJV3Zu6s8kXU1RyTt1grRWyPro8xqh7GlMUZcbDuIi5WxjjP14tCPbcOJNnB7ykhXtTK2nOaJ LsiPtVVp0nFoYz39vy9InMULbbP9yvjLpCuRkOX9jPnm3g==; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=zsh.org; s=rsa-20210803; h=List-Archive:List-Owner:List-Post:List-Unsubscribe: List-Subscribe:List-Help:List-Id:Sender:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Cc:Content-ID:Content-Description:Resent-Date:Resent-From :Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=09EvBXoWT6ze9yr8qJK9ts26wHpV8/vaQOkCm2+wia0=; b=oYmtp3DxbY6EXNOSkhRvzxM3vB eMAeKAApOUadzbaXuDv15ORCq9Gz7bDfFie6csDxPri0BMW57/aNVYtohVTLSVDYDJvCLw4Na0yqq 53n4p5OLewTCeRPKUtGP80Yp4p9aAtVB2JWomZHmIFjiS/M9UhRHGCAPc8+anaIyEnX7oiBk7lA9A a7b7fNYHG6A2pTEMM2iCpOTv1A+9jWoXP7EUpx/BiRH351cC8PPa6gZemskfbISLNuSmIDYhrXGLp vJQuD4PFG4wptppJRWWPhZuTtlRUUZDT72NieEEiAMTzaE4uD8y1UNbGH3SPobkRVz5p+Nv0ibRgD ljKslEeQ==; Received: by zero.zsh.org with local id 1oy2SK-000HeK-Cy; Thu, 24 Nov 2022 02:59:16 +0000 Authentication-Results: zsh.org; iprev=pass (iris.zentaur.org) smtp.remote-ip=198.58.127.206; dkim=pass header.d=zentaur.org header.s=dkim20200120 header.a=rsa-sha256; dmarc=pass header.from=zentaur.org; arc=none Received: from iris.zentaur.org ([198.58.127.206]:35500) by zero.zsh.org with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) id 1oy2Rv-000HMV-Nl; Thu, 24 Nov 2022 02:58:53 +0000 Received: from iris.zentaur.org (localhost [127.0.0.1]) by iris.zentaur.org (Postfix) with ESMTP id 4NHjPQ59BZz3wZj for ; Thu, 24 Nov 2022 02:58:50 +0000 (UTC) Authentication-Results: iris.zentaur.org (amavisd-new); dkim=pass (2048-bit key) reason="pass (just generated, assumed good)" header.d=zentaur.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=zentaur.org; h= content-transfer-encoding:content-type:content-type:in-reply-to :from:from:content-language:references:to:subject:subject :user-agent:mime-version:date:date:message-id; s=dkim20200120; t=1669258727; x=1669262328; bh=BeQE297AsCcrlr52qioCiO2/KAkjRj1G sa5GEpYCxu4=; b=iiFsaOLBoIv5h9eDBUxU4gWvEQyl4Rd8KfsIOvdoVKOyGDE0 +OqEbXzObGmn2MtAA6qF5UNU7C5SgnoEJKVlf9ByB6w6MoFC1c8Ooo1T9s7KMG3F D6Svrjdf/DqtSZZWJe/3A5Gpoo7CICu9bLTqE02ShUA1/X4laVYQFbgWSkiB+gRD z3Iip6bBvVRI9z+9Ovd+n/4GFw460IY7kqgkZrokscxfh0nHqnBZGTErjyxR+Dkr hKm7ll44YeiYkIuJ6u1eYTXnjPJ68QHvyDNOry/+TwcxeVZgus5f9wyfVklJw5fB n0W+efAYI/L//Dozjz2d3QX0L9pO4feaQY5pnQ== X-Virus-Scanned: amavisd-new at iris.zentaur.org Received: from iris.zentaur.org ([127.0.0.1]) by iris.zentaur.org (iris.zentaur.org [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id FyP8izpr0pOl for ; Thu, 24 Nov 2022 02:58:47 +0000 (UTC) Received: from [192.168.78.67] (173-207-53-93.cpe.cableone.net [173.207.53.93]) by iris.zentaur.org (Postfix) with ESMTPSA id 4NHjPM3yStz3wZb for ; Thu, 24 Nov 2022 02:58:47 +0000 (UTC) Message-ID: <4e500f9c-48ef-e1eb-ed7c-5895bd5473ab@zentaur.org> Date: Wed, 23 Nov 2022 20:58:36 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH] zsh/random module [UPDATED] To: zsh-workers@zsh.org References: <741b77be-b679-76cc-f8ec-49c9d89323c1@zentaur.org> <1e8ea669-7a25-b321-6024-72dbc43ac023@zentaur.org> <41205a86-8aad-4821-baa4-1d2ac9bf3c5d@app.fastmail.com> <1b2cafe6-b4b5-c59a-11f3-4dbc1e99e2bc@zentaur.org> <6275a5ac-3a47-f591-7b3c-380ec4fed5ac@zentaur.org> <20221123194658.GM27622@tarpaulin.shahaf.local2> Content-Language: en-US From: Clinton Bunch In-Reply-To: <20221123194658.GM27622@tarpaulin.shahaf.local2> Content-Type: text/plain; charset=UTF-8; format=flowed X-Antivirus: Avast (VPS 221123-4, 11/23/2022), Outbound message X-Antivirus-Status: Clean Content-Transfer-Encoding: quoted-printable X-Seq: 51058 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: , List-Subscribe: , List-Unsubscribe: , List-Post: List-Owner: List-Archive: This is a from memory reply as I'm away from my git repository/code for=20 the holiday. :)=C2=A0 So there may be a follow up. On 11/23/2022 1:46 PM, Daniel Shahaf wrote: > Clinton Bunch wrote on Mon, Nov 07, 2022 at 18:18:15 -0600: > +++ b/Doc/Zsh/mod_random.yo >> @@ -0,0 +1,67 @@ >> +startitem() >> +findex(getrandom) >> +cindex(random data, printing, array) >> +xitem(tt(getrandom) [ tt(-c) var(count) ] [ tt(-r) ] [ tt(-s) var(sca= lar) ] [tt(-i) ] [ tt(-a) var(array) ] [ tt(-L) var(lower_bound) ] [ tt(-= U) var(upper_bound) ]) > Never use xitem() without item() after it. I'm using existing docs as a template as I don't know YODL at all, so=20 I'm not surprised I made mistakes. > > I don't understand this API. > > - Magic numbers: > > + Why should -c default to 8 in the "random bytes" case? Why > shouldn't it default to some other value, or even be required to b= e > specified explicitly by the user? Not sure why 8 was the number I thought of, just that one wasn't=20 particularly useful. > > + Why should -c0 be an error? It should be valid to request an arra= y > of 0=C2=A0random numbers, just like, say, =C2=AByes | head -n 0=C2= =BB. I thought nonsensical options must be an error. > > + Why should -c65 be an error? (Saw your comment elsethread that yo= u > imagine that would suffice. That's not a good reason for this lim= it.) It started as a 256 byte limit from getrandom(). > > + Why should -L$(( 2**32 - 1)) be an error? I should be able to req= uest > a random integer in the range { x | 42 =E2=89=A4 x =E2=89=A4 42 }.= It's perfectly > well-defined. if the difference between lower and upper is zero, you get a divide by=20 zero error unless you special case it, and it never occurred to me that=20 someone would specify that case deliberately > > - Why should -c's default depend on anything at all? You mentioned > downthread you consider making -c1 the default in all cases; that'd = be > better. > > - The builtin can generate either integers or bytes. Most of the API > (the type of "returned random data" that -c deals with, the default > number of those, validity of -L/-U, etc.) is split right along this > line. Shouldn't this become two separate builtins, then? One for > bytes and one for integers? > > (More on this below, at the end of the implementation review.) > > - Interdependencies. This can generate either bytes or unsigned ints, > and bytes can be emitted either in hex, raw, or in decimal, and eith= er > to stdout or to a scalar or to an array=E2=80=A6 but not all of thes= e > combinations are possible. For instance, adding =C2=AB-a=C2=BB to =C2= =ABgetrandom -c 1=C2=BB > changes the output encoding from hex to decimal AND the output chann= el > from stdout to $reply. It seems better to avoid coupling things lik= e > this. > >> +subsect(Parameters) >> + >> +startitem() >> +vindex(SRANDOM) >> +item(tt(SRANDOM)) ( >> +A random positive 32-bit integer between 0 and 4,294,967,295. This p= arameter >> +is read-only. >> +) >> +enditem() >> + >> +subsect(Math Functions) >> + >> +startitem() >> +item(tt(zrandom+LPAR()RPAR())) ( > Doesn't this need a findex()? > > Do other math functions have index entries? And if not (systell() > doesn't), shouldn't they? > >> +++ b/Src/Modules/random.c >> @@ -0,0 +1,675 @@ > There's a whole bunch of orthographic issues in this file (whitespace, > punctuation, spelling, etc.). I won't point them out now since there > are bigger issues. > >> +#include > C99-ism. (zsh is written in C89.) > >> +/* buffer to pre-load integers for SRANDOM to lessen the context swit= ches */ >> +uint32_t rand_buff[8]; > C99-ism (the array element type). I asked about POSIX standards as the dev guide seemed badly out of date=20 in workers/50819 Got this response in workers/50847 On Tue, Oct 25, 2022 at 6:52 PM Clinton Bunch wrote= : > > My personal opinion is that development should use at least the > POSIX-1.2001 standard.=C2=A0 It's 20 years old.=C2=A0 That's surely ol= d enough for > any system still running.=C2=A0 (It's=C2=A0 certainly old enough that = any such > system is not supported by the vendor) OTOH any system not supported by the vendor is exactly one where somebody might be trying to build their own zsh binary. That said, I thought we standardized on c99 rather than c89 quite some=20 time ago. > > Even in C99, wouldn't this require an explicit #include? I guess it > works now due to transitive #include's, but I don't know that it's > guaranteed to work everywhere. > >> +getrandom_buffer(void *buf, size_t len) >> +{ > =E2=8B=AE >> + do { >> +#ifdef HAVE_ARC4RANDOM >> + /* Apparently, the creaters of arc4random didn't believe in errors *= / > Leave out the /ad homines/. > > FWIW, I would sooner guess arc4random() was originally written on > a platform where it /couldn't/ error. > >> + arc4random_buf(buf,len); >> + ret =3D len; >> +#elif defined(HAVE_GETRANDOM) >> + ret=3Dgetrandom(bufptr,(len - val),0); >> +#else >> + ret=3Dread(randfd,bufptr,(len - val)); > The premise of the module is to offer an interface to high-quality > random numbers. Is /dev/urandom, which is the module falls back to > here, necessarily of sufficient quality? If not, shouldn't there be > a way for the module's user to determine the source of randomness? arc4random and getrandom are just more efficient access to the pool of=20 /dev/urandom.=C2=A0=C2=A0 AFAIK it's as good a pseudo-random source as an= y on any=20 platform. > >> +#endif >> + if (ret < 0) { >> + if (errno !=3D EINTR || errflag || retflag || breaks || contflag= ) { >> + zwarn("Unable to get random data: %e.", errno); > Need to set =C2=ABerrno =3D 0=C2=BB first, surely? > >> +/* >> + * Implements the getrandom builtin to return a string of random byte= s of >> + * user-specified length. It either prints them to stdout or returns= them >> + * in a parameter passed on the command line. >> + * >> + */ >> + >> +/**/ >> +static int >> +bin_getrandom(char *name, char **argv, Options ops, int func) >> +{ > =E2=8B=AE >> + bool integer_out =3D false; /* boolean */ > The target audience for comments knows C. So, nuke this comment. > > You might define inline enums here (for ints/bytes, scalar/array/stdout= , > and decimal/hex/raw) to make the code more self-documenting. > > Incidentally, this would also immediately raise the question of why som= e > combinations of these aren't valid (e.g., hex or raw bytes output to an > array). > >> + int i, j; /* loop counters */ >> + >> + /*maximum string length of a 32-bit integer + null */ >> + char int2str[11]; > Have the comment point out that this doesn't include a sign byte? > > FWIW, we have a 64-bit version of this in DIGBUFSIZE. > >> + /* Vailues for -U *nd -L */ >> + zulong upper =3D UINT32_MAX, lower =3D 0; >> + uint32_t diff =3D UINT32_MAX; >> + uint32_t min =3D 0, rej =3D 0; /* Reject below min and count *= / > =C2=ABrej=C2=BB is never read. > >> + bool bound =3D false; /* set if upper or lower bound sp= ecified */ > Why is this variable needed? Having it at all means that passing =C2=AB= -L 0 > -U $((2**32-1))=C2=BB is different to not passing any -L or -U option a= t all, > even though these values are ostensibly the defaults. > >> + if (OPT_ISSET(ops, 'U')) { >> + if (!zstrtoul_underscore(OPT_ARG(ops, 'U'), &upper)) { >> + zwarnnam(name, "Couldn't convert -U %s to a number.", >> + OPT_ARG(ops, 'U')); >> + return 1; >> + } else if (upper > UINT32_MAX || upper < 1) { >> + zwarnnam(name, >> + "Upper bound must be between 1 and %z you specified: %z.", >> + (zlong) UINT32_MAX, (zlong) upper); > If =C2=ABupper > ZLONG_MAX=C2=BB, the downcast to zlong would result in > a negative number. zwarnnam has no zulong format and upper is supposed to be a 32-bit intege= r. > >> + return 1; > =E2=8B=AE >> + if (bound) { > =E2=8B=AE >> + if (lower > upper) { >> + zwarnnam(name, "-U must be larger than -L."); >> + return 1; >> + } >> + >> + diff =3D upper =3D=3D UINT32_MAX && lower =3D=3D 0 ? UINT32_MAX : >> + upper - lower + 1; > How can this possibly be right? As written it means the meaning of > =C2=ABdiff=C2=BB with the default values of =C2=ABupper=C2=BB/=C2=ABlow= er=C2=BB is different to its > meaning in literally every other case. > > Ah, I see. Without special-casing the default values, explicitly > passing =C2=AB-L 0 -U $((2**32 - 1))=C2=BB would incorrectly trigger th= e > zwarnnam() just below, because =C2=ABdiff=C2=BB is a uint32_t. Thus, t= he > special-casing is an incorrect fix to a real issue. > >> + if(!diff) { >> + zwarnnam(name, "Upper and Lower bounds cannot be the same."); >> + return 1; >> + } > This warning will never be printed. [Why? Because =C2=ABdiff=C2=BB is= either > UINT32_MAX, which is true, or =C2=AB(uint32_t)(zulong)(upper - lower + = 1)=C2=BB, > which, taking into account the range checks on =C2=ABupper=C2=BB and =C2= =ABlower=C2=BB, can > only work out to=C2=A00 if =C2=ABupper =3D=3D UINT32_MAX && lower =3D=3D= 0=C2=BB=E2=80=A6 which is > precisely the case that's special-cased in the assignment to =C2=ABdiff= =C2=BB just > above.] > > I suppose it was intended to be printed for =C2=ABgetrandom -L 42 -U 42= =C2=BB. In > practice, that incantation just prints 42. As I mentioned in the API > review, I think that's desired behaviour. In either case, please add > a test. > >> + } >> + >> + if(!OPT_ISSET(ops,'c')) >> + if ((!bound && !integer_out) || arrname) >> + len =3D 8; > So one default value of -c is here and the other is up in the > declaration of =C2=ABlen=C2=BB. That's somewhat confusing. > >> + >> + >> + if (!byte_len) >> + byte_len =3D len; > The condition is always true. > > Didn't you get a compiler warning for this? No.=C2=A0 Not in Developer's Studio, clang, gcc, or HP ANSI C. > >> + >> + >> + if (bound) { >> + pad =3D len / 16 + 1; >> + byte_len =3D (len + pad) * sizeof(uint32_t); > =C2=ABpad=C2=BB should be declared in this scope since it's not used el= sewhere. > Ditto =C2=ABoutbuff=C2=BB below and others. > > Why =C2=ABlen / 16 + 1=C2=BB? I couldn't find any statistical information that would tell me how many=20 numbers were likely to be rejected, so I guessed. >> + } else if (integer_out) >> + byte_len =3D len * sizeof(uint32_t); >> + >> + if (byte_len > 256) >> + byte_len=3D256; >> + > Why not report an error to the user? > > =E2=8B=AE >> + min =3D -diff % diff; > See above about the special casing in the assignment to =C2=ABdiff=C2=BB= . I guess > it affects this line. > > =E2=8B=AE >> + sprintf(int2str, "%u", int_val); > Hold on. %u expects an =C2=ABunsigned int=C2=BB and =C2=ABint_val=C2=BB= is a =C2=ABuint32_t=C2=BB. > So, this line is only correct when =C2=ABunsigned int=C2=BB happens to = be a 32-bit > type=E2=80=A6 while there's nothing stopping that type from being a 64-= bit type, > or even a 16-bit type, and in either case this line would read the wron= g > number of bytes off the varargs. > >> + if (arrname) { >> + array[i]=3Dztrdup(int2str); >> + } else if (scalar && len =3D=3D 1) { >> + setiparam(scalar, int_val); >> + } else { >> + printf("%s ",int2str); >> + } >> + } >> + if (arrname) { >> + setaparam(arrname,array); >> + } else if (!scalar) { >> + printf("\n"); >> + } >> + >> + >> + zfree(buf, byte_len); >> + return 0; >> +} >> + > In the API docs review I proposed to split =C2=ABgetrandom=C2=BB into t= wo separate > builtins. > > Having read this function, I reiterate that proposal. This function, > with all the if/then cases that try to keep track of which of the > function's different modes of operation was requested, is difficult to > read and to modify. > >> + >> +/* >> + * Provides for the SRANDOM parameter and returns an unsigned 32-bit = random >> + * integer. >> + */ >> + >> +/**/ >> +static zlong >> +get_srandom(UNUSED(Param pm)) { >> + >> + if(buf_cnt < 0) { >> + getrandom_buffer((void*) rand_buff,sizeof(rand_buff)); >> + buf_cnt=3D7; > Magic number. You might want =C2=ABsizeof(rand_buff) / sizeof(rand_buf= f[0]) - 1=C2=BB=E2=80=A6 > > =E2=80=A6but it'd be more idiomatic to leave that=C2=A0=C2=AB- 1=C2=BB = out and then change the > postfix decrement to a prefix decrement. That would also be more > consistent with the variable's name. (As written, =C2=ABbuf_cnt =3D=3D= 0=C2=BB means > there's one more zlong in it; it's not a "count" but a "largest valid > index".) > >> + } >> + return rand_buff[buf_cnt--]; >> +} >> + >> +/* >> + * Implements the mathfunc zrandom and returns a random floating-poin= t >> + * number between 0 and 1. Does this by getting a random 32-bt integ= er >> + * and dividing it by UINT32_MAX. >> + * > Presumably this means to guarantee a /uniformly-distributed/ random > floating-point number in the given range. So, why does "getting > a random 32-bit integer and dividing it by UINT32_MAX" actually > implement that guarantee? > > Oh, wait, never mind. The comment is out of date. Fix it. > >> + */ >> + >> +/**/ >> +static mnumber >> +math_zrandom(UNUSED(char *name), UNUSED(int argc), UNUSED(mnumber *ar= gv), >> + UNUSED(int id)) >> +{ >> + mnumber ret; >> + double r; >> + >> + r =3D random_real(); >> + if (r < 0) { >> + zwarnnam(name, "Failed to get sufficient random data."); > Say why. (Probably via errno?) > > Rule of thumb: every error message should include at least one replacea= ble. > > (Further instances of this elsewhere in the file.) > >> +/**/ >> +int >> +setup_(UNUSED(Module m)) >> +{ >> +#ifdef USE_URANDOM >> + /* Check for the existence of /dev/urandom */ >> + >> + struct stat st; >> + >> + if (lstat("/dev/urandom",&st) < 0) { > Why not stat()? Is it appropriate for /dev/urandom to be a symlink? > >> + zwarn("No kernel random pool found."); >> + return 1; >> + } >> + >> + if (!(S_ISCHR(st.st_mode)) ) { >> + zwarn("No kernel random pool found."); >> + return 1; >> + } >> +#endif /* USE_URANDOM */ >> + return 0; >> +} >> +/**/ >> +int >> +_zclz64(uint64_t x) { > =E2=8B=AE >> + if (!(x & 0xFFFFFFFF00000000)) { > Don't you need ZLONG_CONST() here? > >> +/**/ >> +uint64_t >> +random64(void) { >> + uint64_t r; >> + >> + if(getrandom_buffer(&r,sizeof(r)) < 0) { >> + zwarn("zsh/random: Can't get sufficient random data."); >> + return 1; /* 0 will cause loop */ > Where is the docstring of random64() itself? /That/ is what determines > whether or not returning 0 would be correct. > > What loops and why? > > Also, I suspect this name will collide with a third-party function one > day. Recommending to rename. > >> + } >> + >> + return r; >> +} >> + >> +/* Following code is under the below copyright, despite changes for e= rror >> + * handling and removing GCCisms */ >> + > -1. Don't say "Following code"; identify the code explicitly, or bette= r > yet, move it to its own *.c file. Otherwise, any function we might > append to this file would be taken to be covered by this copyright > notice and license statement. > >> +/* >> + * random_real: Generate a stream of bits uniformly at random and >> + * interpret it as the fractional part of the binary expansion of a >> + * number in [0, 1], 0.00001010011111010100...; then round it. >> + */ >> + >> +/**/ >> +double > Need =C2=ABstatic=C2=BB. > > (Further instances of this elsewhere in the file.) > >> +random_real(void) >> +{ > =E2=8B=AE >> +} >> +++ b/Test/V15random.ztst >> @@ -0,0 +1,51 @@ >> +# Tests for the zsh/random module >> +%test >> + getrandom -U 56 >> +1f:Checking if system has a kernel random source >> +?(eval):1: No kernel random pool found. >> + > The test point's code has nothing to do with the test point's > description and expected errput. > > The 'f' flag is why this test point's failure doesn't get reported. The test is supposed to fail. > >> + getrandom -U 64 >> +0:Checking upper Bound is observed >> +*><0-64> >> + >> + tmpmsg=3D"failed" >> + getrandom -L 4 -U 5 -c 16 -a tmpa >> + for i in ${tmpa[@]} >> + do >> + if (( i =3D=3D 5 )); then >> + tmpmsg=3D"success" >> + break >> + fi >> + done > Use ${tmpa[(I)5]} ? > >> + echo $tmpmsg >> +0:Checking that upper bound is inclusive >> +F:Try running the test again: make TESTNUM=3DV15 check > Baby and bathwater. Say what the problem is first and what the > recommended fix is second. In this case, something along the lines > of "This test WILL false positive once every 65536 runs on average. To > rule this out, run the test again." > > Oh, and bump that 16 to something 3 or 4 times as big, because a 1/6553= 6 > chance isn't really enough in a world where automated builds (CI, > distros' QA, etc.) is a thing. > >> +>success >> + >> + echo $(( zrandom() )) >> +0:Checking if zrandom() produces a floating-point number between 0 an= d 1 >> +*>0(|.)[[:digit:]]# > Why is the period optional? Something like "042" shouldn't be accepted= . > >> diff --git a/configure.ac b/configure.ac >> index 074141d38..f1fa01274 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -675,6 +675,7 @@ fi >> AC_CHECK_HEADERS(sys/time.h sys/times.h sys/select.h termcap.h termi= o.h \ >> termios.h sys/param.h sys/filio.h string.h memory.h \ >> limits.h fcntl.h libc.h sys/utsname.h sys/resource.h \ >> + sys/random.h \ > Tabs/spaces are wrong. Also in the next hunk. > >> locale.h errno.h stdio.h stdarg.h varargs.h stdlib.h \ >> unistd.h sys/capability.h \ >> utmp.h utmpx.h sys/types.h pwd.h grp.h poll.h sys/mman.h \ >> @@ -1337,6 +1338,7 @@ AC_CHECK_FUNCS(strftime strptime mktime timeloca= l \ >> cygwin_conv_path \ >> nanosleep \ >> srand_deterministic \ >> + getrandom arc4random \ >> setutxent getutxent endutxent getutent) >> AC_FUNC_STRCOLL >> =20 > Bottom line: > > - Interfacing to kernel random number APIs seems reasonable enough. > > - The API to shell scripts feels unintuitive. > > - As the patch stands, it is not ready to be merged (even assuming > arguendo the API is perfect as-is). > > Thanks, > > Daniel >