diff mbox series

[2/4] idmapd: Add graceful exit and resource cleanup

Message ID 20200722055354.28132-3-nazard@nazar.ca (mailing list archive)
State New, archived
Headers show
Series nfs-utils patches | expand

Commit Message

Doug Nazar July 22, 2020, 5:53 a.m. UTC 
Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 utils/idmapd/idmapd.c | 75 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 10 deletions(-)

Comments

Steve Dickson July 23, 2020, 5:56 p.m. UTC | #1 
Hello,

On 7/22/20 1:53 AM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <nazard@nazar.ca>
> ---
>  utils/idmapd/idmapd.c | 75 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index 491ef54c..dae5e567 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -155,6 +155,7 @@ static void idtonameres(struct idmap_msg *);
>  static void nametoidres(struct idmap_msg *);
>  
>  static int nfsdopen(void);
> +static void nfsdclose(void);
>  static int nfsdopenone(struct idmap_client *);
>  static void nfsdreopen_one(struct idmap_client *);
>  static void nfsdreopen(void);
> @@ -167,6 +168,20 @@ static char *nobodyuser, *nobodygroup;
>  static uid_t nobodyuid;
>  static gid_t nobodygid;
>  static struct event_base *evbase = NULL;
> +static bool signal_received = false;
> +
> +static void
> +sig_die(int signal)
> +{
> +	if (signal_received) {
> +		xlog_warn("forced exiting on signal %d\n", signal);
> +		exit(0);
> +	}
> +
> +	signal_received = true;
> +	xlog_warn("exiting on signal %d\n", signal);
> +	event_base_loopexit(evbase, NULL);
> +}
>  
>  static int
>  flush_nfsd_cache(char *path, time_t now)
> @@ -210,14 +225,15 @@ main(int argc, char **argv)
>  {
>  	int wd = -1, opt, fg = 0, nfsdret = -1;
>  	struct idmap_clientq icq;
> -	struct event *rootdirev, *clntdirev, *svrdirev, *inotifyev;
> -	struct event *initialize;
> +	struct event *rootdirev = NULL, *clntdirev = NULL,
> +		     *svrdirev = NULL, *inotifyev = NULL;
> +	struct event *initialize = NULL;
>  	struct passwd *pw;
>  	struct group *gr;
>  	struct stat sb;
>  	char *xpipefsdir = NULL;
>  	int serverstart = 1, clientstart = 1;
> -	int inotify_fd;
> +	int inotify_fd = -1;
>  	int ret;
>  	char *progname;
>  	char *conf_path = NULL;
> @@ -290,6 +306,9 @@ main(int argc, char **argv)
>  			serverstart = 0;
>  	}
>  
> +	/* Not needed anymore */
> +	conf_cleanup();
> +
I'm a bit confused by this comment... If it is not needed why as the call added?

steved.
>  	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1)
>  		switch (opt) {
>  		case 'v':
> @@ -398,6 +417,9 @@ main(int argc, char **argv)
>  
>  		TAILQ_INIT(&icq);
>  
> +		signal(SIGINT, sig_die);
> +		signal(SIGTERM, sig_die);
> +
>  		/* These events are persistent */
>  		rootdirev = evsignal_new(evbase, SIGUSR1, dirscancb, &icq);
>  		if (rootdirev == NULL)
> @@ -435,7 +457,25 @@ main(int argc, char **argv)
>  	if (event_base_dispatch(evbase) < 0)
>  		xlog_err("main: event_dispatch returns errno %d (%s)",
>  			    errno, strerror(errno));
> -	/* NOTREACHED */
> +
> +	nfs4_term_name_mapping();
> +	nfsdclose();
> +
> +	if (inotifyev)
> +		event_free(inotifyev);
> +	if (inotify_fd != -1)
> +		close(inotify_fd);
> +
> +	if (initialize)
> +		event_free(initialize);
> +	if (rootdirev)
> +		event_free(rootdirev);
> +	if (clntdirev)
> +		event_free(clntdirev);
> +	if (svrdirev)
> +		event_free(svrdirev);
> +	event_base_free(evbase);
> +
>  	return 1;
>  }
>  
> @@ -760,6 +800,19 @@ out:
>  	event_add(ic->ic_event, NULL);
>  }
>  
> +static void
> +nfsdclose_one(struct idmap_client *ic)
> +{
> +	if (ic->ic_event) {
> +		event_free(ic->ic_event);
> +		ic->ic_event = NULL;
> +	}
> +	if (ic->ic_fd != -1) {
> +		close(ic->ic_fd);
> +		ic->ic_fd = -1;
> +	}
> +}
> +
>  static void
>  nfsdreopen_one(struct idmap_client *ic)
>  {
> @@ -769,12 +822,7 @@ nfsdreopen_one(struct idmap_client *ic)
>  		xlog_warn("ReOpening %s", ic->ic_path);
>  
>  	if ((fd = open(ic->ic_path, O_RDWR, 0)) != -1) {
> -		if (ic->ic_event && event_initialized(ic->ic_event)) {
> -			event_del(ic->ic_event);
> -			event_free(ic->ic_event);
> -		}
> -		if (ic->ic_fd != -1)
> -			close(ic->ic_fd);
> +		nfsdclose_one(ic);
>  
>  		ic->ic_fd = fd;
>  		ic->ic_event = event_new(evbase, ic->ic_fd, EV_READ, nfsdcb, ic);
> @@ -807,6 +855,13 @@ nfsdopen(void)
>  		    nfsdopenone(&nfsd_ic[IC_IDNAME]) == 0) ? 0 : -1);
>  }
>  
> +static void
> +nfsdclose(void)
> +{
> +	nfsdclose_one(&nfsd_ic[IC_NAMEID]);
> +	nfsdclose_one(&nfsd_ic[IC_IDNAME]);
> +}
> +
>  static int
>  nfsdopenone(struct idmap_client *ic)
>  {
>
Doug Nazar July 23, 2020, 6:25 p.m. UTC | #2 
On 2020-07-23 13:56, Steve Dickson wrote:
> @@ -290,6 +306,9 @@ main(int argc, char **argv)
>>   			serverstart = 0;
>>   	}
>>   
>> +	/* Not needed anymore */
>> +	conf_cleanup();
>> +
> I'm a bit confused by this comment... If it is not needed why as the call added?

Sorry, I should have been a bit more verbose in the comment. I meant 
that we didn't need access to the config file anymore (we've already 
looked up everything) and can free those resources early.

Perhaps /* Config not needed anymore */ or something.

Doug
Steve Dickson July 23, 2020, 7:16 p.m. UTC | #3 
On 7/23/20 2:25 PM, Doug Nazar wrote:
> On 2020-07-23 13:56, Steve Dickson wrote:
>> @@ -290,6 +306,9 @@ main(int argc, char **argv)
>>>               serverstart = 0;
>>>       }
>>>   +    /* Not needed anymore */
>>> +    conf_cleanup();
>>> +
>> I'm a bit confused by this comment... If it is not needed why as the call added?
> 
> Sorry, I should have been a bit more verbose in the comment. I meant that we didn't need access to the config file anymore (we've already looked up everything) and can free those resources early.
> 
> Perhaps /* Config not needed anymore */ or something.
Ok.. got it... I'll make it work... 

steved.

> 
> Doug
>
diff mbox series

Patch

diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index 491ef54c..dae5e567 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -155,6 +155,7 @@  static void idtonameres(struct idmap_msg *);
 static void nametoidres(struct idmap_msg *);
 
 static int nfsdopen(void);
+static void nfsdclose(void);
 static int nfsdopenone(struct idmap_client *);
 static void nfsdreopen_one(struct idmap_client *);
 static void nfsdreopen(void);
@@ -167,6 +168,20 @@  static char *nobodyuser, *nobodygroup;
 static uid_t nobodyuid;
 static gid_t nobodygid;
 static struct event_base *evbase = NULL;
+static bool signal_received = false;
+
+static void
+sig_die(int signal)
+{
+	if (signal_received) {
+		xlog_warn("forced exiting on signal %d\n", signal);
+		exit(0);
+	}
+
+	signal_received = true;
+	xlog_warn("exiting on signal %d\n", signal);
+	event_base_loopexit(evbase, NULL);
+}
 
 static int
 flush_nfsd_cache(char *path, time_t now)
@@ -210,14 +225,15 @@  main(int argc, char **argv)
 {
 	int wd = -1, opt, fg = 0, nfsdret = -1;
 	struct idmap_clientq icq;
-	struct event *rootdirev, *clntdirev, *svrdirev, *inotifyev;
-	struct event *initialize;
+	struct event *rootdirev = NULL, *clntdirev = NULL,
+		     *svrdirev = NULL, *inotifyev = NULL;
+	struct event *initialize = NULL;
 	struct passwd *pw;
 	struct group *gr;
 	struct stat sb;
 	char *xpipefsdir = NULL;
 	int serverstart = 1, clientstart = 1;
-	int inotify_fd;
+	int inotify_fd = -1;
 	int ret;
 	char *progname;
 	char *conf_path = NULL;
@@ -290,6 +306,9 @@  main(int argc, char **argv)
 			serverstart = 0;
 	}
 
+	/* Not needed anymore */
+	conf_cleanup();
+
 	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1)
 		switch (opt) {
 		case 'v':
@@ -398,6 +417,9 @@  main(int argc, char **argv)
 
 		TAILQ_INIT(&icq);
 
+		signal(SIGINT, sig_die);
+		signal(SIGTERM, sig_die);
+
 		/* These events are persistent */
 		rootdirev = evsignal_new(evbase, SIGUSR1, dirscancb, &icq);
 		if (rootdirev == NULL)
@@ -435,7 +457,25 @@  main(int argc, char **argv)
 	if (event_base_dispatch(evbase) < 0)
 		xlog_err("main: event_dispatch returns errno %d (%s)",
 			    errno, strerror(errno));
-	/* NOTREACHED */
+
+	nfs4_term_name_mapping();
+	nfsdclose();
+
+	if (inotifyev)
+		event_free(inotifyev);
+	if (inotify_fd != -1)
+		close(inotify_fd);
+
+	if (initialize)
+		event_free(initialize);
+	if (rootdirev)
+		event_free(rootdirev);
+	if (clntdirev)
+		event_free(clntdirev);
+	if (svrdirev)
+		event_free(svrdirev);
+	event_base_free(evbase);
+
 	return 1;
 }
 
@@ -760,6 +800,19 @@  out:
 	event_add(ic->ic_event, NULL);
 }
 
+static void
+nfsdclose_one(struct idmap_client *ic)
+{
+	if (ic->ic_event) {
+		event_free(ic->ic_event);
+		ic->ic_event = NULL;
+	}
+	if (ic->ic_fd != -1) {
+		close(ic->ic_fd);
+		ic->ic_fd = -1;
+	}
+}
+
 static void
 nfsdreopen_one(struct idmap_client *ic)
 {
@@ -769,12 +822,7 @@  nfsdreopen_one(struct idmap_client *ic)
 		xlog_warn("ReOpening %s", ic->ic_path);
 
 	if ((fd = open(ic->ic_path, O_RDWR, 0)) != -1) {
-		if (ic->ic_event && event_initialized(ic->ic_event)) {
-			event_del(ic->ic_event);
-			event_free(ic->ic_event);
-		}
-		if (ic->ic_fd != -1)
-			close(ic->ic_fd);
+		nfsdclose_one(ic);
 
 		ic->ic_fd = fd;
 		ic->ic_event = event_new(evbase, ic->ic_fd, EV_READ, nfsdcb, ic);
@@ -807,6 +855,13 @@  nfsdopen(void)
 		    nfsdopenone(&nfsd_ic[IC_IDNAME]) == 0) ? 0 : -1);
 }
 
+static void
+nfsdclose(void)
+{
+	nfsdclose_one(&nfsd_ic[IC_NAMEID]);
+	nfsdclose_one(&nfsd_ic[IC_IDNAME]);
+}
+
 static int
 nfsdopenone(struct idmap_client *ic)
 {