[PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff
Maxim Dounin
mdounin at mdounin.ru
Fri Dec 22 21:30:26 UTC 2023
Hello!
On Fri, Dec 22, 2023 at 05:52:30PM +0400, Sergey Kandaurov wrote:
> On Thu, Dec 21, 2023 at 07:14:40PM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Thu, Dec 21, 2023 at 05:37:02PM +0400, Sergey Kandaurov wrote:
> >
> > > > On 16 Dec 2023, at 06:57, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > > >
> > > > Hello!
> > > >
> > > > On Sat, Dec 09, 2023 at 08:42:11AM +0000, J Carter wrote:
> > > >
> > > >> On Sat, 09 Dec 2023 07:46:10 +0000
> > > >> J Carter <jordanc.carter at outlook.com> wrote:
> > > >>
> > > >>> # HG changeset patch
> > > >>> # User J Carter <jordanc.carter at outlook.com>
> > > >>> # Date 1702101635 0
> > > >>> # Sat Dec 09 06:00:35 2023 +0000
> > > >>> # Node ID 1a77698f82d2580aa8b8f62ce89b4dbb6d678c5d
> > > >>> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f
> > > >>> Win32: extended ngx_random range to 0x7fffffff
> > > >>>
> > > >>> rand() is used on win32. RAND_MAX is implementation defined. win32's is
> > > >>> 0x7fff.
> > > >>>
> > > >>> Existing uses of ngx_random rely upon 0x7fffffff range provided by
> > > >>> posix implementations of random().
> > > >>>
> > > >>> diff -r d9275e982a71 -r 1a77698f82d2 src/os/win32/ngx_win32_config.h
> > > >>> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 +0000
> > > >>> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 06:00:35 2023 +0000
> > > >>> @@ -280,7 +280,9 @@
> > > >>>
> > > >>> #define NGX_HAVE_GETADDRINFO 1
> > > >>>
> > > >>> -#define ngx_random rand
> > > >>> +#define ngx_random \
> > > >>> + ((rand() << 16) | (rand() << 1) | (rand() >> 14))
> > > >>> +
> > > >>> #define ngx_debug_init()
> > > >>>
> > > >>>
> > > >>
> > > >> ^ my mistake - copying error..
> > > >>
> > > >> # HG changeset patch
> > > >> # User J Carter <jordanc.carter at outlook.com>
> > > >> # Date 1702111094 0
> > > >> # Sat Dec 09 08:38:14 2023 +0000
> > > >> # Node ID 10ef59a412a330872fc6d46de64f42e32e997b3a
> > > >> # Parent d9275e982a7188a1ea7855295ffa93362ea9830f
> > > >> Win32: extended ngx_random range to 0x7fffffff
> > > >
> > > > Nitpicking:
> > > >
> > > > Win32: extended ngx_random() range to 0x7fffffff.
> > > >
> > > >>
> > > >> rand() is used on win32. RAND_MAX is implementation defined. win32's is
> > > >> 0x7fff.
> > > >>
> > > >> Existing uses of ngx_random rely upon 0x7fffffff range provided by
> > > >> posix implementations of random().
> > > >
> > > > Thanks for catching this.
> > > >
> > > > As far as I can see, the only module which actually relies on the
> > > > range is the random index module. Relying on the ngx_random()
> > > > range generally looks wrong to me, and we might want to change the
> > > > code to don't. OTOH, it's the only way to get a completely
> > > > uniform distribution, and that's what the module tries to do. As
> > > > such, it might be good enough to preserve it as is, at least till
> > > > further changes to ngx_random().
> > > >
> > > > Either way, wider range for ngx_random() should be beneficial in
> > > > other places.
> > > >
> > > >>
> > > >> diff -r d9275e982a71 -r 10ef59a412a3 src/os/win32/ngx_win32_config.h
> > > >> --- a/src/os/win32/ngx_win32_config.h Sat Dec 09 05:09:07 2023 +0000
> > > >> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 +0000
> > > >> @@ -280,7 +280,9 @@
> > > >>
> > > >> #define NGX_HAVE_GETADDRINFO 1
> > > >>
> > > >> -#define ngx_random rand
> > > >> +#define ngx_random() \
> > > >
> > > > Nitpicking: the "\" character should be at the 79th column (in
> > > > some files at 78th). This ensures that a diff won't wrap on a
> > > > standard 80-column terminal.
> > > >
> > > >> + ((rand() << 16) | (rand() << 1) | (rand() >> 14))
> > > >> +
> > > >> #define ngx_debug_init()
> > > >
> > > > Using "|" for random numbers looks utterly wrong to me, even if
> > > > ORed values are guaranteed to not interfere.
> > > >
> > > > I would rather use "^", and avoid dependency on the particular
> > > > value of RAND_MAX (other than POSIX-required minimum of 32767) by
> > > > using something like
> > > >
> > > > 0x7fffffff & ((rand() << 16) ^ (rand() << 8) ^ rand())
> > > >
> > > > with proper typecasts.
> > > >
> > > > Something like this should work:
> > > >
> > > > diff --git a/src/os/win32/ngx_win32_config.h b/src/os/win32/ngx_win32_config.h
> > > > --- a/src/os/win32/ngx_win32_config.h
> > > > +++ b/src/os/win32/ngx_win32_config.h
> > > > @@ -280,7 +280,11 @@ typedef int sig_atomic_t
> > > >
> > > > #define NGX_HAVE_GETADDRINFO 1
> > > >
> > > > -#define ngx_random rand
> > > > +#define ngx_random() \
> > > > + ((long) (0x7fffffff & ( ((uint32_t) rand() << 16) \
> > > > + ^ ((uint32_t) rand() << 8) \
> > > > + ^ ((uint32_t) rand()) )))
> > > > +
> > > > #define ngx_debug_init()
> > > >
> > > >
> > >
> > > Nitpicking: you might want to re-align the "^" operator to the first
> > > symbol of the left-hand operand (similar to NGX_CONF_TAKE1234, or
> > > even NGX_UNIX_ADDRSTRLEN). Other than that, it looks good.
> >
> > It's intentionally aligned this way to simplify reading. Such
> > style is occasionally used in complex macro definitions, see
> > ngx_mp4_get_32value() or ngx_proxy_protocol_parse_uint32() for
> > some examples.
> >
> > Just for completeness, below is the updated patch.
> >
> > # HG changeset patch
> > # User J Carter <jordanc.carter at outlook.com>
> > # Date 1702111094 0
> > # Sat Dec 09 08:38:14 2023 +0000
> > # Node ID 92923ac5ea2a395774b28460f07d0fd2e1a2de24
> > # Parent cc16989c6d61385027c1ebfd43929f8369fa5f62
> > Win32: extended ngx_random() range to 0x7fffffff.
> >
> > rand() is used on win32. RAND_MAX is implementation defined. win32's is
> > 0x7fff.
>
> I'd unwrap this line to look more pleasant, it fits into 80 columns.
That's from the J Carter's original commit log, unmodified. While
I can't say it cannot be improved, I've tried to refrain from
changes.
Also, wrapping here looks correct to me, 72 chars line width is
perfectly fine unless there are reasons to use something
different, and it is in line with wrapping of the rest of the
commit log.
>
> >
> > Existing uses of ngx_random() rely upon 0x7fffffff range provided by
> > POSIX implementations of random().
> >
> > diff -r cc16989c6d61 -r 92923ac5ea2a src/os/win32/ngx_win32_config.h
> > --- a/src/os/win32/ngx_win32_config.h Sat Dec 16 03:40:01 2023 +0400
> > +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 +0000
> > @@ -280,7 +280,11 @@ typedef int sig_atomic_t
> >
> > #define NGX_HAVE_GETADDRINFO 1
> >
> > -#define ngx_random rand
> > +#define ngx_random() \
> > + ((long) (0x7fffffff & ( ((uint32_t) rand() << 16) \
>
> If I'm not mistaken, indentation is now 5 spaces.
> Otherwise, looks good.
Yep, thanks for catching, slipped in from an earlier version of
the patch.
Fixed and pushed to http://mdounin.ru/hg/nginx, thanks for
looking.
[...]
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list