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.

> +#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)
>
>
> --
> Sergey Kandaurov
>
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org