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

[Ahmad Gani <reyuki@gnuweeb.org>]

On Fri, Aug 29, 2025 at 11:36 AM Ammar Faizi wrote:
> On Fri, Aug 29, 2025 at 10:36:25AM +0700, Ahmad Gani wrote:
> > +     close(e->udp_fd);
>
> Again, please use __sys_close() instead of close() for all code in
> src/gwproxy/. This rule applies to future patches too.

Oops, sorry for the oversight. I will replace it.

> > +             default:
> > +                     assert(0);
> > +                     break;
> > +             }
> > +
> > +             txid = (uint16_t)rand();
> > +             r = gwdns_build_query(txid, name, af, e->payload, sizeof(e->payload));
>
> This part is problematic when compiled without debug mode because
> 'assert(0)' is actually a macro that does nothing when compiled
> with -DNDEBUG. The compiler says:
>
>   gcc -ggdb3 -rdynamic -O2 -fpic -fPIC -shared -o libgwpsocks5.so src/gwproxy/socks5.c.o -lpthread -lanl
>   src/gwproxy/dns.c: In function ‘gwp_dns_queue’:
>   src/gwproxy/dns.c:969:21: warning: ‘af’ may be used uninitialized [-Wmaybe-uninitialized]
>     969 |                 r = gwdns_build_query(txid, name, af, e->payload, sizeof(e->payload));
>         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   src/gwproxy/dns.c:937:13: note: ‘af’ was declared here
>     937 |         int af;
>         |             ^~
>
> I suggest to return -EINVAL instead of break.

The function returns NULL in case of error.

> Don't forget to free associated resources if needed. You can keep assert(0)
> for the debug mode. Keep in mind that assert() is a macro that disappears
> entirely when compiled with -DNDEBUG.

I was not aware of the warning because I compiled it in debug mode.
Thanks for the notice. I will fix it.

> I have added a GitHub CI build using './configure --debug' to cover
> breakage for both non-debug and debug build cases. You can push your
> changes to your GitHub fork and make sure everything is green before
> sending the patches to the list.

Okay, I will make sure it's all green first.

> You need to rebase your work on top of b2a7a2d33ba867 ("github: Test
> build with --debug flag"), obviously.

FYI: This patchset is already rebased on top of b2a7a2d33ba867.

--
Ahmad Gani