On Fri, 2023-08-18 at 18:10 +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 18, 2023 at 04:14:01PM +0200, Thomas Haller wrote:
> > Hi Pablo,
> >
> > On Fri, 2023-08-18 at 11:57 +0200, Pablo Neira Ayuso wrote:
> > >
> > > > - struct protoent *p;
> > > > -
> > > > if (!nft_output_numeric_proto(octx)) {
> > > > - p = getprotobynumber(mpz_get_uint8(expr-
> > > > >value));
> > > > - if (p != NULL) {
> > > > - nft_print(octx, "%s", p->p_name);
> > > > + char name[1024];
> > >
> > > Is there any definition that could be used instead of 1024. Same
> > > comment for all other hardcoded buffers. Or maybe add a
> > > definition
> > > for
> > > this?
> >
> > Added defines instead. See v3.
> >
> > [...]
> >
> > > > #include <nftables.h>
> > > > #include <utils.h>
> > > > @@ -105,3 +106,90 @@ int round_pow_2(unsigned int n)
> > > > {
> > > > return 1UL << fls(n - 1);
> > > > }
> > > > +
> > >
> > > Could you move this new code to a new file instead of utils.c? We
> > > are
> > > slowing moving towards GPLv2 or any later for new code. Probably
> > > netdb.c or pick a better name that you like.
> >
> > This request leaves me with a lot of choices. I made them, but I
> > guess
> > you will have something to say about it. See v3.
> >
> > >
> > > > +bool nft_getprotobynumber(int proto, char *out_name, size_t
> > > > name_len)
> > > > +{
> > > > + const struct protoent *result;
> > > > +
> > > > +#if HAVE_DECL_GETPROTOBYNUMBER_R
> > > > + struct protoent result_buf;
> > > > + char buf[2048];
> > > > + int r;
> > > > +
> > > > + r = getprotobynumber_r(proto,
> > > > + &result_buf,
> > > > + buf,
> > > > + sizeof(buf),
> > > > + (struct protoent **) &result);
> > > > + if (r != 0 || result != &result_buf)
> > > > + result = NULL;
> > > > +#else
> > > > + result = getprotobynumber(proto);
> > > > +#endif
> > >
> > > I'd suggest wrap this code with #ifdef's in a helper function.
> >
> > I don't understand. nft_getprotobynumber() *is* that helper
> > function to
> > wrap the #if. This point is not addressed by v3 (??).
>
> I mean, something like a smaller function:
>
> static struct __nft_getprotobynumber(...)
>
> that is wraps this code above and it returns const struct protoent.
> This helper function is called from nft_getprotobynumber().
>
> But it is fine as it is, this is just a bit of bike shedding.
>
I see. Yes, possible.
I don't find that very useful, because the idea is that
nft_getprotobyname() etc. wraps the underlying libc functions and
contains the #if parts. And there shall be no other place that directly
call getprotobyname()/getprotobyname_r(). So `__nft_getprotobynumber()`
would be a static function with only one caller (and no plan to ever
have more).
Thomas
