Message ID | 1355311690-15723-1-git-send-email-pontus.fuchs@gmail.com |
---|---|
State | Rejected |
Headers | show |
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.
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
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)