Patchwork [v2] Allow PID file to be written in foreground mode

login
register
mail settings
Submitter Pontus Fuchs
Date Dec. 12, 2012, 11:28 a.m.
Message ID <1355311690-15723-1-git-send-email-pontus.fuchs@gmail.com>
Download mbox | patch
Permalink /patch/205485/
State Rejected
Headers show

Comments

Pontus Fuchs - Dec. 12, 2012, 11:28 a.m.
I wanted hostapd to write a PID file in foreground mode but I found
that it couldn't. It will silently ignore the -P option when -B is
not specified on the command line.

Fix this by moving the PID file writing from os_daemonize to a new
function.

Signed-hostap: Pontus Fuchs <pontus.fuchs@gmail.com>
---

V2:
* Fix a typo in commit msg.
* Fix the change to wpa_cli.c which accidently got chewed when moving
  the patch to a newer tree. Compilation was still fine so I didn't
  see it.

 hostapd/hostapd_cli.c           |    3 ++-
 hostapd/main.c                  |    3 ++-
 src/utils/os.h                  |   10 ++++++++--
 src/utils/os_internal.c         |   12 +++++++++---
 src/utils/os_none.c             |    8 +++++++-
 src/utils/os_unix.c             |   12 ++++++++----
 src/utils/os_win32.c            |    8 +++++++-
 wpa_supplicant/wpa_cli.c        |    4 +++-
 wpa_supplicant/wpa_priv.c       |    3 ++-
 wpa_supplicant/wpa_supplicant.c |    8 ++++----
 10 files changed, 52 insertions(+), 19 deletions(-)
Jouni Malinen - Dec. 23, 2012, 10:46 a.m.
On Wed, Dec 12, 2012 at 12:28:10PM +0100, Pontus Fuchs wrote:
> I wanted hostapd to write a PID file in foreground mode but I found
> that it couldn't. It will silently ignore the -P option when -B is
> not specified on the command line.
> 
> Fix this by moving the PID file writing from os_daemonize to a new
> function.

What's the use case for this? The previous pair of os_daemonize and
os_daemonize_terminate was matching pretty nicely while this this
combination of three functions and especially the name of
os_daemonize_terminate in this case is not as clean.

> V2:
> * Fix a typo in commit msg.
> * Fix the change to wpa_cli.c which accidently got chewed when moving
>   the patch to a newer tree. Compilation was still fine so I didn't
>   see it.

Hmm.. There is still something strange with wpa_cli.c.


> diff --git a/src/utils/os.h b/src/utils/os.h
> @@ -77,10 +77,9 @@ int os_gmtime(os_time_t t, struct os_tm *tm);
> -int os_daemonize(const char *pid_file);
> +int os_daemonize();

That should be os_daemonize(void) (and same for the eloop*.c files).

> +int os_write_pid_file(const char *pid_file)
> +{
>  	if (pid_file) {
>  		FILE *f = fopen(pid_file, "w");
>  		if (f) {
>  			fprintf(f, "%u\n", getpid());
>  			fclose(f);
> +			return 0;
>  		}
>  	}
> -
> -	return -0;
> +	return -1;

Should this really return -1 (error?) if pid_file is not set. The
changes you made for the callers seem to be ignoring the return value
completely, so this could as well be a void function..

> diff --git a/wpa_supplicant/wpa_cli.c b/wpa_supplicant/wpa_cli.c
> @@ -3482,8 +3482,10 @@ int main(int argc, char *argv[])
> -		if (daemonize && os_daemonize(pid_file))
> +		if (daemonize && os_daemonize()) {
>  			return -1;
> +			os_write_pid_file(pid_file);
> +		}

??

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -2400,10 +2400,10 @@ int wpa_supplicant_driver_init(struct wpa_supplicant *wpa_s)
> -static int wpa_supplicant_daemon(const char *pid_file)
> +static int wpa_supplicant_daemon()

(void)

Though, with what remains in the function, you might as well remove this
unnecessary wrapper completely.
Pontus Fuchs - Jan. 2, 2013, 9:08 a.m.
On 2012-12-23 11:46, Jouni Malinen wrote:
> On Wed, Dec 12, 2012 at 12:28:10PM +0100, Pontus Fuchs wrote:
>> I wanted hostapd to write a PID file in foreground mode but I found
>> that it couldn't. It will silently ignore the -P option when -B is
>> not specified on the command line.
>>
>> Fix this by moving the PID file writing from os_daemonize to a new
>> function.
>
> What's the use case for this? The previous pair of os_daemonize and
> os_daemonize_terminate was matching pretty nicely while this this
> combination of three functions and especially the name of
> os_daemonize_terminate in this case is not as clean.

The purpose was to have hostapd write it's pid when started through 
logwrapper. I ended up solving my problem in another way so please drop 
this.

//Pontus

Patch

diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
index 633c13d..cf6d88b 100644
--- a/hostapd/hostapd_cli.c
+++ b/hostapd/hostapd_cli.c
@@ -1096,8 +1096,9 @@  int main(int argc, char *argv[])
 		}
 	}
 
-	if (daemonize && os_daemonize(pid_file))
+	if (daemonize && os_daemonize())
 		return -1;
+	os_write_pid_file(pid_file);
 
 	if (interactive)
 		hostapd_cli_interactive();
diff --git a/hostapd/main.c b/hostapd/main.c
index 56f0002..d13a742 100644
--- a/hostapd/main.c
+++ b/hostapd/main.c
@@ -451,10 +451,11 @@  static int hostapd_global_run(struct hapd_interfaces *ifaces, int daemonize,
 	}
 #endif /* EAP_SERVER_TNC */
 
-	if (daemonize && os_daemonize(pid_file)) {
+	if (daemonize && os_daemonize()) {
 		perror("daemon");
 		return -1;
 	}
+	os_write_pid_file(pid_file);
 
 	eloop_run();
 
diff --git a/src/utils/os.h b/src/utils/os.h
index ad20834..18d8dc6 100644
--- a/src/utils/os.h
+++ b/src/utils/os.h
@@ -77,10 +77,9 @@  int os_gmtime(os_time_t t, struct os_tm *tm);
 
 /**
  * os_daemonize - Run in the background (detach from the controlling terminal)
- * @pid_file: File name to write the process ID to or %NULL to skip this
  * Returns: 0 on success, -1 on failure
  */
-int os_daemonize(const char *pid_file);
+int os_daemonize();
 
 /**
  * os_daemonize_terminate - Stop running in the background (remove pid file)
@@ -89,6 +88,13 @@  int os_daemonize(const char *pid_file);
 void os_daemonize_terminate(const char *pid_file);
 
 /**
+ * os_write_pid_file - Write current process ID to pid_file
+ * @pid_file: File name to write the process ID to or %NULL to skip this
+ * Returns: 0 on success, -1 on failure
+ */
+int os_write_pid_file(const char *pid_file);
+
+/**
  * os_get_random - Get cryptographically strong pseudo random data
  * @buf: Buffer for pseudo random data
  * @len: Length of the buffer
diff --git a/src/utils/os_internal.c b/src/utils/os_internal.c
index e4b7fdb..3ebae06 100644
--- a/src/utils/os_internal.c
+++ b/src/utils/os_internal.c
@@ -82,22 +82,28 @@  int os_gmtime(os_time_t t, struct os_tm *tm)
 }
 
 
-int os_daemonize(const char *pid_file)
+int os_daemonize()
 {
 	if (daemon(0, 0)) {
 		perror("daemon");
 		return -1;
 	}
 
+	return 0;
+}
+
+
+int os_write_pid_file(const char *pid_file)
+{
 	if (pid_file) {
 		FILE *f = fopen(pid_file, "w");
 		if (f) {
 			fprintf(f, "%u\n", getpid());
 			fclose(f);
+			return 0;
 		}
 	}
-
-	return -0;
+	return -1;
 }
 
 
diff --git a/src/utils/os_none.c b/src/utils/os_none.c
index cabf73b..cf02476 100644
--- a/src/utils/os_none.c
+++ b/src/utils/os_none.c
@@ -38,7 +38,13 @@  int os_gmtime(os_time_t t, struct os_tm *tm)
 }
 
 
-int os_daemonize(const char *pid_file)
+int os_daemonize()
+{
+	return -1;
+}
+
+
+int os_write_pid_file(const char *pid_file)
 {
 	return -1;
 }
diff --git a/src/utils/os_unix.c b/src/utils/os_unix.c
index 23a93be..1248a17 100644
--- a/src/utils/os_unix.c
+++ b/src/utils/os_unix.c
@@ -153,7 +153,7 @@  static int os_daemon(int nochdir, int noclose)
 #endif /* __APPLE__ */
 
 
-int os_daemonize(const char *pid_file)
+int os_daemonize()
 {
 #if defined(__uClinux__) || defined(__sun__)
 	return -1;
@@ -162,17 +162,21 @@  int os_daemonize(const char *pid_file)
 		perror("daemon");
 		return -1;
 	}
+	return 0;
+#endif /* defined(__uClinux__) || defined(__sun__) */
+}
 
+int os_write_pid_file(const char *pid_file)
+{
 	if (pid_file) {
 		FILE *f = fopen(pid_file, "w");
 		if (f) {
 			fprintf(f, "%u\n", getpid());
 			fclose(f);
+			return 0;
 		}
 	}
-
-	return -0;
-#endif /* defined(__uClinux__) || defined(__sun__) */
+	return -1;
 }
 
 
diff --git a/src/utils/os_win32.c b/src/utils/os_win32.c
index 163cebe..b5a01a9 100644
--- a/src/utils/os_win32.c
+++ b/src/utils/os_win32.c
@@ -105,13 +105,19 @@  int os_gmtime(os_time_t t, struct os_tm *tm)
 }
 
 
-int os_daemonize(const char *pid_file)
+int os_daemonize()
 {
 	/* TODO */
 	return -1;
 }
 
 
+int os_write_pid_file(const char *pid_file)
+{
+	return -1;
+}
+
+
 void os_daemonize_terminate(const char *pid_file)
 {
 }
diff --git a/wpa_supplicant/wpa_cli.c b/wpa_supplicant/wpa_cli.c
index 61bb7fd..785b784 100644
--- a/wpa_supplicant/wpa_cli.c
+++ b/wpa_supplicant/wpa_cli.c
@@ -3482,8 +3482,10 @@  int main(int argc, char *argv[])
 			}
 		}
 
-		if (daemonize && os_daemonize(pid_file))
+		if (daemonize && os_daemonize()) {
 			return -1;
+			os_write_pid_file(pid_file);
+		}
 
 		if (action_file)
 			wpa_cli_action(ctrl_conn);
diff --git a/wpa_supplicant/wpa_priv.c b/wpa_supplicant/wpa_priv.c
index ad6a080..149ae89 100644
--- a/wpa_supplicant/wpa_priv.c
+++ b/wpa_supplicant/wpa_priv.c
@@ -1008,8 +1008,9 @@  int main(int argc, char *argv[])
 		interfaces = iface;
 	}
 
-	if (daemonize && os_daemonize(pid_file))
+	if (daemonize && os_daemonize())
 		goto out;
+	os_write_pid_file(pid_file);
 
 	eloop_register_signal_terminate(wpa_priv_terminate, NULL);
 	eloop_run();
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index 784a471..74bec00 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -2400,10 +2400,10 @@  int wpa_supplicant_driver_init(struct wpa_supplicant *wpa_s)
 }
 
 
-static int wpa_supplicant_daemon(const char *pid_file)
+static int wpa_supplicant_daemon()
 {
 	wpa_printf(MSG_DEBUG, "Daemonize..");
-	return os_daemonize(pid_file);
+	return os_daemonize();
 }
 
 
@@ -3246,9 +3246,9 @@  int wpa_supplicant_run(struct wpa_global *global)
 {
 	struct wpa_supplicant *wpa_s;
 
-	if (global->params.daemonize &&
-	    wpa_supplicant_daemon(global->params.pid_file))
+	if (global->params.daemonize && wpa_supplicant_daemon())
 		return -1;
+	os_write_pid_file(global->params.pid_file);
 
 	if (global->params.wait_for_monitor) {
 		for (wpa_s = global->ifaces; wpa_s; wpa_s = wpa_s->next)