On 06/26/2007 05:42 PM, Daniel Stenberg wrote:
> We should make sure the old command line option still works though and
> not simply rename it to the new. That is easily made by simply accepting
> --krb4 as well as --krb.

OK, I guess. Actually, since krb is a unique abbreviation to krb4, it would
also work to just leave it at krb4, but not advertise it as krb4 any more.

> The only minor problems I have with your patch are:
>
> 1) it unconditionally makes name resolving use AI_CANONNAME to
> getaddrinfo()
> which will add unnecessary reverse lookups to libcurl resolves that have
> no need for them - I would rather like to see that bit get switched on
> dynamicly only when that data is actually needed.

OK, how about adding "if(conn->data->set.krb)" in front of that? It's needed
for proper gssapi host name principals. I assume the krb option will only be
set if you're using FTP anyway. Of course I'm assuming that conn->data will
never be NULL, so maybe "if(conn->data && conn->data->set.krb)"? Or maybe
there's a also a way to find out if FTP is being used for that lookup?

> 2) + /* FIXME: need to free this eventually */
> + ai->ai_canonname = strdup(he->h_name);
>
> ... this sounds and looks like a memory leak to me!

Well, yes, I didn't really want to search the whole code for places ai might be
freed. I figured that since the lookups are cached, it wouldn't be a very bad
leak. If it's only freed by Curl_freeaddrinfo(), I guess freeing it just before
free(ai) would work:

  if(ai->ai_canonname)
   free(ai->ai_canonname)

In any case, I could make these changes and resend the whole patch, but I doubt
that's really necessary, is it? (not that I mind - I just don't like sending
large amounts of text to mailing lists)

  - Thomas J. Moore
    UITS/Research Technology/Research Storage
    Indiana University/IUPUI
Received on 2007-06-27