[Bug-gnulib] Re: getaddrinfo

[Simon Josefsson]

Paul Eggert <address@hidden> writes:

> Simon Josefsson <address@hidden> writes:
>
>> +# define EAI_BADFLAGS         -1    /* Invalid value for `ai_flags' field.  
>> */
>
> "-1" should be parenthesized here, and similarly for the other macros.
>
>> +  if (hints &&
>> +      hints->ai_protocol != SOCK_STREAM &&
>> +      hints->ai_protocol != SOCK_DGRAM)
>> +      /* FIXME: Support other protocols. */
>> +    return EAI_SERVICE; /* FIXME: Better return code? */
>> ...
>> +  if (servname)
>> +    {
>> +      /* FIXME: Use getservbyname_r if available. */
>> +      se = getservbyname (servname,
>> +                      hints->ai_protocol == SOCK_DGRAM ? "udp" : "tcp");
>> +      if (!se)
>> +    return EAI_SERVICE;
>> +    }
>
> This dumps core if (servname && !hints).

Oops, now it reads:

  if (servname)
    {
      const char *proto =
        (hints && hints->ai_protocol == SOCK_DGRAM) ? "udp" : "tcp";

      /* FIXME: Use getservbyname_r if available. */
      se = getservbyname (servname, proto);

> Also, I don't see why the code cares about hints->ai_protocol when
> !servname.

Shouldn't it do that?  The code doesn't care much about
hints->ai_protocol now, except checking for udp and tcp.  Do you want
to remove that check?  I think the code should ideally care about
ai_protocol more, not less.

> freeaddrinfo has a memory leak: it doesn't free the struct sockaddr_in
> or struct sockaddr_in6 structure allocated by getaddrinfo.  To fix
> this I would modify getaddrinfo to invoke malloc just once, to
> allocate all the memory that it needs, rather than invoking it twice.

You mean the addrinfo structs should be allocated as malloc (sizeof
(*res) + sizeof (struct sockaddr_in)) and make the ai_addr pointer
point to after the addrinfo?  Sounds doable.

>> +const char *
>> +gai_strerror (int code)
>> +{
>> +  size_t i;
>> +  for (i = 0; i < sizeof (values) / sizeof (values[0]); ++i)
>> +    if (values[i].code == code)
>> +      return _(values[i].msg);
>
> A small point: since you specify the encoding for "code" you could
> make gai_strerror an O(1) operation (by using "code" as an index
> directly into a table) rather than O(N).

Yes.

OTOH, if later on, some system happen to define EAI_*, but is missing
the getaddrinfo implementation, it might be better to use the system's
EAI_* values inside gnulib's getaddrinfo.  The #define EAI's should
then be wrapped around HAVE_DECL_EAI_FOO etc.  In that case, the O(N)
approach is probably required.  Also, it can be error prone to keep
the error array and the #define's in sync, especially since "code" is
typically negative, so it can be easy to do a manual +-1 error when
keeping this in sync (OK, not a strong argument).

OTTH, if some system define only some of the POSIX EAI_* errors, it
will be complicated to map the remaining errors into unique values.
So it might be better to do #undef EAI_AGAIN and define the errors
into some known values.

Finally, perhaps a better approach: we should use gai_strerror.c from
glibc CVS instead.  I copied the above code from that file, but
keeping the files synchronized seem better.  Then if glibc is improved
to do O(1), so will gnulib.

Thanks.