From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server-vie001.gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=new2025; t=1757089577; bh=DmOk67nLW+Mi532GPePuQvGpS6EArHhdQ8SJPmuOvIY=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type:Content-Transfer-Encoding:Message-ID:Date:From: Reply-To:Subject:To:Cc:In-Reply-To:References:Resent-Date: Resent-From:Resent-To:Resent-Cc:User-Agent:Content-Type: Content-Transfer-Encoding; b=Qr/ww7cEeuLX1aOCWkjFrFzu4J/K8RNvIZf8Fefz8mGZfSCsECblyEmkyTsPcjVSk O+TGT8P6Lcc6FRDeNtvNwBmKcosIDn/bAoyKPd8VaiCZmXT3roxMX1tMDBaFxPwriF OhIYYnBU6LlatiPn/tTTzHNQK1S10QZ88eA2Hbirq1nPvKIFAEYpcWXbOZIHsdMe/0 HkQdGJemF3K0nXaQkkRdj2QODZYTkwt2jOaqOEzj5iIiD7PjquY1BH5ayqvyK87qCZ RnAEmREeqQKgHStN80bNrggDfOuoOf0PsURxOlmCuk8l738wEG4yDfaamd9A0id5p1 Gh1ZuqJ7fVinA== Received: from mail-pl1-f170.google.com (mail-pl1-f170.google.com [209.85.214.170]) by server-vie001.gnuweeb.org (Postfix) with ESMTPSA id 3488C3127A2F for ; Fri, 5 Sep 2025 16:26:17 +0000 (UTC) Received: by mail-pl1-f170.google.com with SMTP id d9443c01a7336-24458272c00so27142905ad.3 for ; Fri, 05 Sep 2025 09:26:17 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCWx2amK+NozZP4uWzXfI1FFj0RKCFxwOpKPcRicCIj2Q6nm4BVtwgNZRn1ynULXwz0fSbMf@vger.gnuweeb.org X-Gm-Message-State: AOJu0Yxz9w8w2anLkHaHOrCq0mm2rT/9DacEfMqzKjXPtfKfExsPwdRu doOrQ8t+xuFsryCrasknFWBQLIkLKQN+Ntjb7Wyt8Vf5W3zXvS9IIXus6BrxgYLePEE8Nvcm8J4 UWfXtPjsstMudHpLuZb3lj7U8bTCEp2Q= X-Google-Smtp-Source: AGHT+IEo0pjqSzMJErq80SJ3zSDbXo3pc+xKJOcCjwUl1ueh4LGbrP7p9KFKsC4DKbRoCXscX0u0GDqJdMepG1h6rT0= X-Received: by 2002:a17:903:2309:b0:248:ef1f:bbbe with SMTP id d9443c01a7336-249446d2b42mr314699235ad.0.1757089575474; Fri, 05 Sep 2025 09:26:15 -0700 (PDT) MIME-Version: 1.0 References: <20250829075557.598176-1-reyuki@gnuweeb.org> <20250829075557.598176-3-reyuki@gnuweeb.org> In-Reply-To: <20250829075557.598176-3-reyuki@gnuweeb.org> From: Alviro Iskandar Setiawan Date: Fri, 5 Sep 2025 23:26:01 +0700 X-Gmail-Original-Message-ID: X-Gm-Features: Ac12FXwElLjfZQYyXsz0llEjtxNN42afZoLk_KNnYXGrfoIBqU_wRqHMC2CgM-c Message-ID: Subject: Re: [PATCH gwproxy v8 2/2] gwproxy: refactor code base to add experimental raw DNS backend To: Ahmad Gani Cc: Ammar Faizi , "GNU/Weeb Mailing List" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: 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=E2=80=AFPM Ahmad Gani wrote: > +void cp_nsaddr(struct gwp_dns_ctx *ctx, struct gwp_sockaddr *addr, uint8= _t *addrlen) > +{ > + *addr =3D ctx->ns_addr; > + *addrlen =3D 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 =3D __sys_recvfrom( > + e->udp_fd, buff, sizeof(buff), 0, > + &ctx->ns_addr.sa, (socklen_t *)&ctx->ns_addrlen > + ); > + if (r <=3D 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 =3D convert_str_to_ssaddr(cfg->ns_addr_str, &ctx->ns_addr, 53); > + if (r) > goto out_destroy_mutex; > + ctx->ns_addrlen =3D ctx->ns_addr.sa.sa_family =3D=3D AF_INET > + ? sizeof(ctx->ns_addr.i4) > + : sizeof(ctx->ns_addr.i6); > + ctx->entry_cap =3D DEFAULT_ENTRIES_CAP; > + ctx->entries =3D 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 =3D (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 =3D gcp->gde; > + struct gwp_dns_ctx *dctx; > + struct gwp_sockaddr addr; > + uint8_t addrlen; > + ssize_t r; > + > + dctx =3D w->ctx->dns; > + cp_nsaddr(dctx, &addr, &addrlen); > + r =3D __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 =3D __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.