Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Steve Dickson <SteveD@redhat.com>]

Chuck Lever wrote:
> On Tue, Jul 15, 2008 at 2:21 AM, Neil Brown <neilb@suse.de> wrote:

>> If an NFS server has no network connectivity when it reboots,
>> it will block in sm-notify waiting for DNS lookup for a potentially
>> large number of hosts.  This is not helpful and just annoys the
>> sysadmin.
>>
>> So do the DNS lookup in the backgrounded phase of sm-notify,
>> before sending off the NOTIFY requests.
> 
> Good idea.  A couple of minor comments below.
> 

> 
> 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.
> 
> [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].
> 

>>                        /* Set the timeout for this call, using an
>> @@ -532,15 +546,6 @@ get_hosts(const char *dirname)
> 
> You should probably fix the documenting comment for get_hosts() --
> "convert them to host entries" might be more precise.
> 


I believe the following patch address those minor comments... true?

Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index bb67c37..2fc8ec6 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -76,7 +76,7 @@ static int		log_syslog = 0;
 
 static unsigned int	nsm_get_state(int);
 static void		notify(void);
-static void		notify_host(int, struct nsm_host *);
+static int		notify_host(int, struct nsm_host *);
 static void		recv_reply(int);
 static void		backup_hosts(const char *, const char *);
 static void		get_hosts(const char *);
@@ -286,7 +286,13 @@ notify(void)
 			hp = hosts;
 			hosts = hp->next;
 
-			notify_host(sock, hp);
+			if (notify_host(sock, hp)){
+				unlink(hp->path);
+				free(hp->name);
+				free(hp->path);
+				free(hp);
+				continue;
+			}
 
 			/* Set the timeout for this call, using an
 			   exponential timeout strategy */
@@ -318,7 +324,7 @@ notify(void)
 /*
  * Send notification to a single host
  */
-void
+int
 notify_host(int sock, struct nsm_host *host)
 {
 	static unsigned int	xid = 0;
@@ -331,6 +337,15 @@ notify_host(int sock, struct nsm_host *host)
 	if (!host->xid)
 		host->xid = xid++;
 
+	if (hp->ai == NULL) {
+		hp->ai = host_lookup(AF_UNSPEC, hp->name);
+		if (hp->ai == NULL)
+			nsm_log(LOG_WARNING,
+				"%s doesn't seem to be a valid address,"
+				" skipped", hp->name);
+		return 1;
+	}
+
 	memset(msgbuf, 0, sizeof(msgbuf));
 	p = msgbuf;
 	*p++ = htonl(host->xid);
@@ -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;
@@ -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;
 }
 
 /*
@@ -504,7 +522,7 @@ backup_hosts(const char *dirname, const char *bakname)
 }
 
 /*
- * Get all entries from sm.bak and convert them to host names
+ * Get all entries from sm.bak and convert them to host entries
  */
 static void
 get_hosts(const char *dirname)
@@ -532,15 +550,6 @@ get_hosts(const char *dirname)
 		if (stat(path, &stb) < 0)
 			continue;
 
-		host->ai = host_lookup(AF_UNSPEC, de->d_name);
-		if (! host->ai) {
-			nsm_log(LOG_WARNING,
-				"%s doesn't seem to be a valid address, skipped",
-				de->d_name);
-			unlink(path);
-			continue;
-		}
-
 		host->last_used = stb.st_mtime;
 		host->timeout = NSM_TIMEOUT;
 		host->path = strdup(path);