On Tue, Jun 28, 2022 at 04:24:58PM +0400, Roman Arutyunyan wrote:
> On Thu, Jun 16, 2022 at 07:17:13PM +0400, Sergey Kandaurov wrote:
> >
> > > On 23 Feb 2022, at 08:11, Ruslan Ermilov <ru@nginx.com> wrote:
> > >
> > > src/core/ngx_resolver.c | 38 +++++++++++++++++++++++++++++++++++++-
> > > src/core/ngx_resolver.h | 5 +++++
> > > 2 files changed, 42 insertions(+), 1 deletions(-)
> > >
> > >
> > > # HG changeset patch
> > > # User Ruslan Ermilov <ru@nginx.com>
> > > # Date 1645589387 -10800
> > > # Wed Feb 23 07:09:47 2022 +0300
> > > # Node ID 8db4bbd67840e8bebb23f9c6d10c0f633552e616
> > > # Parent 1c19779448db2309d607c74e2628ff98f84569ff
> > > The "sort=" parameter of the "resolver" directive.
> > >
> >
> > IMHO, the name isn't enough self-documenting in this context.
> > It can be misinterpreted as sorting addresses, not families.
> > I'd prefer the parameter name "prefer" here.
>
> Jftr, we had a discussion about this. I like the new name. There's no
> sorting here.
>
> > > diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> > > --- a/src/core/ngx_resolver.c
> > > +++ b/src/core/ngx_resolver.c
> > > @@ -266,6 +266,27 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > > }
> > > #endif
> > >
> > > + if (ngx_strncmp(names[i].data, "sort=", 5) == 0) {
> > > +
> > > + if (ngx_strcasecmp(&names[i].data[5], (u_char *) "ipv4") == 0) {
> >
> > The same question raises about case-sensitivity.
> > I prefer to make it case-sensitive, like other similar directive parameters.
> >
> > > + r->sort = NGX_RESOLVE_A_FIRST;
> > > +
> > > +#if (NGX_HAVE_INET6)
> > > + } else if (ngx_strcasecmp(&names[i].data[5], (u_char *) "ipv6")
> > > + == 0)
> > > + {
> > > + r->sort = NGX_RESOLVE_AAAA_FIRST;
> > > +#endif
> > > +
> > > + } else {
> > > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > > + "invalid parameter: %V", &names[i]);
> > > + return NULL;
> > > + }
> > > +
> > > + continue;
> > > + }
> > > +
> >
> > What about to put the whole block under NGX_HAVE_INET6?
> > If built without ipv6 support it makes same little sense
> > as for "ipv6" and "ipv4" parameters that already there.
> >
> > > ngx_memzero(&u, sizeof(ngx_url_t));
> > >
> > > u.url = names[i];
> > > @@ -4253,7 +4274,22 @@ ngx_resolver_export(ngx_resolver_t *r, n
> > > }
> > >
> > > i = 0;
> > > - d = rotate ? ngx_random() % n : 0;
> > > +
> > > + if (r->sort == NGX_RESOLVE_A_FIRST) {
> > > + d = 0;
> > > +
> > > +#if (NGX_HAVE_INET6)
> > > + } else if (r->sort == NGX_RESOLVE_AAAA_FIRST) {
> > > + d = rn->naddrs6;
> > > +
> > > + if (d == n) {
> > > + d = 0;
> > > + }
> > > +#endif
> > > +
> > > + } else {
> > > + d = rotate ? ngx_random() % n : 0;
> > > + }
> > >
> > > if (rn->naddrs) {
> > > j = rotate ? ngx_random() % rn->naddrs : 0;
> > > diff --git a/src/core/ngx_resolver.h b/src/core/ngx_resolver.h
> > > --- a/src/core/ngx_resolver.h
> > > +++ b/src/core/ngx_resolver.h
> > > @@ -36,6 +36,9 @@
> > >
> > > #define NGX_RESOLVER_MAX_RECURSION 50
> > >
> > > +#define NGX_RESOLVE_A_FIRST 1
> > > +#define NGX_RESOLVE_AAAA_FIRST 2
> > > +
> >
> > I'd adjust name for better sorting.
> >
> > >
> > > typedef struct ngx_resolver_s ngx_resolver_t;
> > >
> > > @@ -185,6 +188,8 @@ struct ngx_resolver_s {
> > > ngx_queue_t addr6_expire_queue;
> > > #endif
> > >
> > > + ngx_uint_t sort;
> > > +
> >
> > Since the previous patch introduces ipv4/ipv6 bit fields,
> > it makes sense to make this one a bit field as well.
> >
> > > time_t resend_timeout;
> > > time_t tcp_timeout;
> > > time_t expire;
> > >
> >
> > diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> > --- a/src/core/ngx_resolver.c
> > +++ b/src/core/ngx_resolver.c
> > @@ -227,6 +227,7 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > }
> >
> > #if (NGX_HAVE_INET6)
> > +
> > if (ngx_strncmp(names[i].data, "ipv4=", 5) == 0) {
> >
> > if (ngx_strcmp(&names[i].data[5], "on") == 0) {
> > @@ -260,6 +261,24 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> >
> > continue;
> > }
> > +
> > + if (ngx_strncmp(names[i].data, "prefer=", 7) == 0) {
> > +
> > + if (ngx_strcmp(&names[i].data[7], "ipv4") == 0) {
> > + r->prefer = NGX_RESOLVE_PREFER_A;
> > +
> > + } else if (ngx_strcmp(&names[i].data[7], "ipv6") == 0) {
> > + r->prefer = NGX_RESOLVE_PREFER_AAAA;
> > +
> > + } else {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "invalid parameter: %V", &names[i]);
> > + return NULL;
> > + }
> > +
> > + continue;
> > + }
> > +
> > #endif
> >
> > ngx_memzero(&u, sizeof(ngx_url_t));
> > @@ -4250,7 +4269,22 @@ ngx_resolver_export(ngx_resolver_t *r, n
> > }
> >
> > i = 0;
> > - d = rotate ? ngx_random() % n : 0;
> > +
> > + if (r->prefer == NGX_RESOLVE_PREFER_A) {
> > + d = 0;
> > +
>
> If we put "prefer=" handler under NGX_HAVE_INET6, the block above should
> also go under NGX_HAVE_INET6. All 3 blocks could even be substituted with
> a switch for simplicity.
I agree it will never match in the no-inet6 case.
Please apply on top off:
diff -r 5210f97325ed src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c Tue Jun 28 19:36:18 2022 +0400
+++ b/src/core/ngx_resolver.c Tue Jun 28 19:58:26 2022 +0400
@@ -4270,19 +4270,24 @@ ngx_resolver_export(ngx_resolver_t *r, n
i = 0;
- if (r->prefer == NGX_RESOLVE_PREFER_A) {
- d = 0;
+ switch (r->prefer) {
#if (NGX_HAVE_INET6)
- } else if (r->prefer == NGX_RESOLVE_PREFER_AAAA) {
+ case NGX_RESOLVE_PREFER_A:
+ d = 0;
+ break;
+
+ case NGX_RESOLVE_PREFER_AAAA:
d = rn->naddrs6;
if (d == n) {
d = 0;
}
+
+ break;
#endif
- } else {
+ default:
d = rotate ? ngx_random() % n : 0;
}
>
> > +#if (NGX_HAVE_INET6)
> > + } else if (r->prefer == NGX_RESOLVE_PREFER_AAAA) {
> > + d = rn->naddrs6;
> > +
> > + if (d == n) {
> > + d = 0;
> > + }
> > +#endif
> > +
> > + } else {
> > + d = rotate ? ngx_random() % n : 0;
> > + }
> >
> > if (rn->naddrs) {
> > j = rotate ? ngx_random() % rn->naddrs : 0;
> > diff --git a/src/core/ngx_resolver.h b/src/core/ngx_resolver.h
> > --- a/src/core/ngx_resolver.h
> > +++ b/src/core/ngx_resolver.h
> > @@ -36,6 +36,9 @@
> >
> > #define NGX_RESOLVER_MAX_RECURSION 50
> >
> > +#define NGX_RESOLVE_PREFER_A 1
> > +#define NGX_RESOLVE_PREFER_AAAA 2
> > +
> >
> > typedef struct ngx_resolver_s ngx_resolver_t;
> >
> > @@ -175,6 +178,8 @@ struct ngx_resolver_s {
> > ngx_queue_t srv_expire_queue;
> > ngx_queue_t addr_expire_queue;
> >
> > + unsigned prefer:2;
> > +
> > unsigned ipv4:1;
> >
> > #if (NGX_HAVE_INET6)
> >
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
