Re: [PATCH gwproxy v8 2/2] gwproxy: refactor code base to add experimental raw DNS backend

[Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>]

Having #ifdef CONFIG_RAW_DNS all over the place is horrible. Please do
something else to reduce them. CTRL + F, "def CONFIG_RAW_DNS", 24
matches. We're not taking this.

On Fri, Aug 29, 2025 at 2:56 PM Ahmad Gani wrote:
> +void cp_nsaddr(struct gwp_dns_ctx *ctx, struct gwp_sockaddr *addr, uint8_t *addrlen)
> +{
> +       *addr = ctx->ns_addr;
> +       *addrlen = ctx->ns_addrlen;
> +}

Why is this exposed globally? I don't think it's worth a function
call. Better be inlined.

> +int gwp_dns_process(struct gwp_dns_ctx *ctx, struct gwp_dns_entry *e)
> +{
> +       struct gwdns_addrinfo_node *ai;
> +       uint8_t buff[UDP_MSG_LIMIT];
> +       ssize_t r;
> +
> +       r = __sys_recvfrom(
> +               e->udp_fd, buff, sizeof(buff), 0,
> +               &ctx->ns_addr.sa, (socklen_t *)&ctx->ns_addrlen
> +       );
> +       if (r <= 0)
> +               return (int)r;

The subsequent code calls gwdns_parse_query(), and you noted that a
return value of 0 indicates success, and by the end of
gwp_dns_process(), it returns what was returned by
gwdns_parse_query(). That's it. It means, if successful, it returns 0.
What happens if __sys_recvfrom() returns 0? Doesn't that mean it's
also considered successful? If so, why?

> +       if (cfg->use_raw_dns) {
> +#ifdef CONFIG_RAW_DNS
> +       r = convert_str_to_ssaddr(cfg->ns_addr_str, &ctx->ns_addr, 53);
> +       if (r)
>                 goto out_destroy_mutex;
> +       ctx->ns_addrlen = ctx->ns_addr.sa.sa_family == AF_INET
> +                       ? sizeof(ctx->ns_addr.i4)
> +                       : sizeof(ctx->ns_addr.i6);
> +       ctx->entry_cap = DEFAULT_ENTRIES_CAP;
> +       ctx->entries = malloc(ctx->entry_cap * sizeof(*ctx->entries));
> +       if (!ctx->entries)
> +               goto out_destroy_mutex;
> +#endif
> +       } else {

Bad indentation. Apart from bad indentation, if the malloc() call
fails, r still contains 0, and you goto out_destroy_mutex, returning 0
on failure. Shouldn't you set -ENOMEM instead?

>  void gwp_dns_ctx_free(struct gwp_dns_ctx *ctx)
>  {
> -       free_workers(ctx);
> +       if (ctx->cfg.use_raw_dns) {
> +#ifdef CONFIG_RAW_DNS
> +       free_all_queued_entries(ctx);
> +#endif
> +       } else {

Bad indentation.

> +               txid = (uint16_t)rand();

Why do you use a random number for the txid? I don't think rand()
guarantees uniqueness. Also, I don't find the srand() call anywhere.
That's equivalent to having srand(1).

What about using a simple incrementing counter instead (which
guarantees the uniqueness)?
It will indeed reset after 65535, but that should give enough room to
complete prior DNS requests first. With a simple counter, the reuse of
an ID will always be 65535 queries away.

>  struct gwp_dns_entry {
> +#ifdef CONFIG_RAW_DNS
> +       uint32_t                idx;
> +       int                     udp_fd;

AFAIU, when using the raw DNS feature you bring. ev_fd is not used,
instead you use udp_fd. So I think it is not worth adding extra unused
space. Better be an anonymous union if the name needs to be different.
No?

> +#ifdef CONFIG_RAW_DNS
> +static int arm_poll_for_raw_dns_query(struct gwp_wrk *w,
> +                                       struct gwp_conn_pair *gcp)
> +{
> +       struct gwp_dns_entry *gde = gcp->gde;
> +       struct gwp_dns_ctx *dctx;
> +       struct gwp_sockaddr addr;
> +       uint8_t addrlen;
> +       ssize_t r;
> +
> +       dctx = w->ctx->dns;
> +       cp_nsaddr(dctx, &addr, &addrlen);
> +       r = __sys_sendto(
> +               gde->udp_fd, gde->payload, gde->payloadlen, MSG_NOSIGNAL,
> +               &addr.sa, addrlen
> +       );
> +
> +       return (int)r;
> +}
> +#endif

Why is this function named arm_poll_for_raw_dns_query()? There is no
poll-arming here. You're simply calling __sys_sendto(). Please write a
function name based on what it does. It's confusing.

> +       if (w->ctx->cfg.use_raw_dns) {
> +#ifdef CONFIG_RAW_DNS
> +       arm_poll_for_raw_dns_query(w, gcp);
> +
> +       r = __sys_epoll_ctl(w->ep_fd, EPOLL_CTL_ADD, gde->udp_fd, &ev);
>         if (unlikely(r))
> -               return r;
> +               return (int)r;
> +#endif
> +       } else {

Bad indentation.

> -       gwp_dns_entry_put(gde);
> +       if (w->ctx->cfg.use_raw_dns) {
> +#ifdef CONFIG_RAW_DNS
> +       gwp_dns_raw_entry_free(w->ctx->dns, gde);
> +#endif

Bad indentation. Why are there many of those?

> +       if (gcp->gde) {
> +               if (w->ctx->cfg.use_raw_dns) {
> +#ifdef CONFIG_RAW_DNS
> +               gwp_dns_raw_entry_free(w->ctx->dns, gcp->gde);
> +#endif
> +               } else {

Bad indentation.