On Wed, Jul 16, 2008 at 10:41 PM, Neil Brown <neilb@xxxxxxx> wrote:
> On Wednesday July 16, SteveD@xxxxxxxxxx wrote:
>>
>> >
>> > Arguably it would be cleaner architecturally if the DNS lookup were in
>> > notify_host(). Since doing it in notify() means you will be looking
>> > up these addresses on retries, maybe the host_lookup() call should be
>> > integrated into the "If we retransmitted 4 times" logic. That would
>> > be an opportunity to fix the addrinfo leak in there.
>
> What addrinfo leak is that?
>
>> >
>> > [Note that freeaddrinfo(3) simply walks the list of addrinfo
>> > structures and frees them. By setting ai_next to NULL in some of the
>> > addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
>> > will never find them].
>> >
>
> I assume we are talking about the code in notify_host in the
> if (host->retries >= 4) {
> branch.
>
> What the code is doing is taking the first addrinfo from the host->ai
> list, and moving it to the end - effectively rotating the list.
> This shouldn't change what freeaddrinfo will do.
>
> One of us is missing something.
Even after staring at this for half an hour last week, it looked to me
like you were chopping off the last addrinfo in the list each time
through the "retries" loop. I could hope for something a little more
straightforward to do the address rotation.
It would be nice to consolidate the acquisition and freeing of the
addrinfo structs for each host so the lifetime of the results is made
clear. Moving the host_lookup() into notify_host() does help with
that.
There is another addrinfo leak in notify(), I think. In the "Bind
source IP if provided on command line" paragraph, host_lookup() is
called and the address is copied, but that list of addrinfo structs is
never freed, is it?
>> I believe the following patch address those minor comments... true?
>
> Except for:
>
>> @@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
>> while ( *next )
>> next = & (*next)->ai_next;
>> *next = hold;
>> - hold->ai_next = NULL;
>> memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
>> addr_set_port(&host->addr, 0);
>> host->retries = 0;
>
> which I think is wrong, and wondering why:
>
>> @@ -394,7 +408,11 @@ notify_host(int sock, struct nsm_host *host)
>> }
>> len = (p - msgbuf) << 2;
>>
>> - sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest));
>> + if (sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest)) < 0)
>> + nsm_log(LOG_WARNING, "Sending Reboot Notification to "
>> + "'%s' failed: errno %d (%s)", host->name, errno, strerror(errno));
>> +
>> + return 0;
>> }
Yes, it's good to check for a return code here; but I'm wondering why
change it in this patch?
>>
>> /*
>
> Yes, I believe it addresses those comments.
>
> Acked-by: NeilBrown <neilb@xxxxxxx>
>
> Thanks!
> NeilBrown
--
Chuck Lever
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
