From b3e2f3700ef4dd15056d7608a105ef29903fb19e Mon Sep 17 00:00:00 2001 From: Simo Piiroinen Date: Mon, 7 Nov 2016 10:09:44 +0200 Subject: [PATCH] [usb-moded] Fix issues on exit path. MER#1694 Exiting from mainloop does not work as intended, looks like handle_exit() is meant to be called from main() function after mainloop is stopped. What happens in practice is that handle_exit() gets called from signal handler and it makes exit() without ever returning to main() function - which means some cleanup tasks are skipped. In general: Make cleanup tasks happen on the same logical level where initialization is made, re-order init/cleanup tasks according to what functional inter dependencies exist and add comments describing why things are done when they are done. Remove handle_exit() function and distribute work it used to do among: - usb_moded_stop() - exiting from mainloop - usb_moded_cleanup() - releasing usb-mode related state/config data - main() - cleaning up init steps made from main() To make possible ordering issues more visible, refuse to send usb-moded dbus signals in situations where the service name is not owned by the ubs-moded process. Signed-off-by: Simo Piiroinen --- src/usb_moded-dbus.c | 58 +++++++-- src/usb_moded.c | 304 ++++++++++++++++++++++++++++--------------- src/usb_moded.h | 5 +- 3 files changed, 251 insertions(+), 116 deletions(-) diff --git a/src/usb_moded-dbus.c b/src/usb_moded-dbus.c index 137e0d3..8d5c5a3 100644 --- a/src/usb_moded-dbus.c +++ b/src/usb_moded-dbus.c @@ -45,6 +45,8 @@ #define INIT_DONE_MATCH "type='signal',interface='"INIT_DONE_INTERFACE"',member='"INIT_DONE_SIGNAL"'" static DBusConnection *dbus_connection_sys = NULL; +static gboolean have_service_name = FALSE; + extern gboolean rescue_mode; /** @@ -53,7 +55,12 @@ extern gboolean rescue_mode; static void usb_moded_send_config_signal(const char *section, const char *key, const char *value) { log_debug(USB_MODE_CONFIG_SIGNAL_NAME ": %s %s %s\n", section, key, value); - if (dbus_connection_sys) + if( !have_service_name ) + { + log_err("config notification without service: [%s] %s=%s", + section, key, value); + } + else if (dbus_connection_sys) { DBusMessage* msg = dbus_message_new_signal(USB_MODE_OBJECT, USB_MODE_INTERFACE, USB_MODE_CONFIG_SIGNAL_NAME); if (msg) { @@ -536,6 +543,8 @@ gboolean usb_moded_dbus_init_service(void) log_debug("DBUS ERROR: %s, %s \n", error.name, error.message); goto EXIT; } + log_debug("claimed name %s", USB_MODE_SERVICE); + have_service_name = TRUE; /* everything went fine */ status = TRUE; @@ -544,23 +553,42 @@ gboolean usb_moded_dbus_init_service(void) return status; } +/** Release "com.meego.usb_moded" D-Bus Service Name + */ +static void usb_moded_dbus_cleanup_service(void) +{ + if( !have_service_name ) + goto EXIT; + + have_service_name = FALSE; + log_debug("release name %s", USB_MODE_SERVICE); + + if( dbus_connection_sys && + dbus_connection_get_is_connected(dbus_connection_sys) ) + { + dbus_bus_release_name(dbus_connection_sys, USB_MODE_SERVICE, NULL); + } + +EXIT: + return; +} + /** * Clean up the dbus connections on exit * */ void usb_moded_dbus_cleanup(void) { - /* clean up system bus connection */ - if (dbus_connection_sys != NULL) - { - if( dbus_connection_get_is_connected(dbus_connection_sys) ) { - // FIXME: do we want to do this? It is pretty useless on exit path. - dbus_bus_release_name(dbus_connection_sys, USB_MODE_SERVICE, NULL); - } - dbus_connection_remove_filter(dbus_connection_sys, msg_handler, NULL); - dbus_connection_unref(dbus_connection_sys); - dbus_connection_sys = NULL; - } + /* clean up system bus connection */ + if (dbus_connection_sys != NULL) + { + usb_moded_dbus_cleanup_service(); + + dbus_connection_remove_filter(dbus_connection_sys, msg_handler, NULL); + + dbus_connection_unref(dbus_connection_sys), + dbus_connection_sys = NULL; + } } /** @@ -575,6 +603,12 @@ static int usb_moded_dbus_signal(const char *signal_type, const char *content) int result = 1; DBusMessage* msg = 0; + if( !have_service_name ) + { + log_err("sending signal without service: %s(%s)", + signal_type, content); + goto EXIT; + } if(!dbus_connection_sys) { log_err("Dbus system connection broken!\n"); diff --git a/src/usb_moded.c b/src/usb_moded.c index 7942fe5..9962920 100644 --- a/src/usb_moded.c +++ b/src/usb_moded.c @@ -61,7 +61,9 @@ /* global definitions */ -GMainLoop *mainloop = NULL; +static int usb_moded_exitcode = EXIT_FAILURE; +static GMainLoop *usb_moded_mainloop = NULL; + extern const char *log_name; extern int log_level; extern int log_type; @@ -626,20 +628,7 @@ static void usb_moded_init(void) #ifdef APP_SYNC readlist(diag_mode); - /* If usb-moded happens to crash, it could leave appsync processes - * running. To make sure things are in the order expected by usb-moded - * force stopping of appsync processes during usb-moded startup. - * - * The exception is: When usb-moded starts as a part of bootup. Then - * we can be relatively sure that usb-moded has not been running yet - * and therefore no appsync processes have been started and we can - * skip the blocking ipc required to stop the appsync systemd units. */ - if( init_done_p() ) - { - log_warning("usb-moded started after init-done; forcing appsync stop"); - appsync_stop(TRUE); - } -#endif /* APP_SYNC */ +#endif /* always read dyn modes even if appsync is not used */ modelist = read_mode_list(diag_mode); @@ -662,6 +651,32 @@ static void usb_moded_init(void) /* TODO: add more start-up clean-up and init here if needed */ } +/** Release resources allocated by usb_moded_init() + */ +static void usb_moded_cleanup(void) +{ + /* Undo usb_moded_module_ctx_init() */ + usb_moded_module_ctx_cleanup(); + + /* Undo trigger_init() */ + trigger_stop(); + + /* Undo read_mode_list() */ + free_mode_list(modelist); + +#ifdef APP_SYNC + /* Undo readlist() */ + free_appsync_list(); +#endif + + /* Release dynamic memory */ + free(current_mode.module), + current_mode.module = 0; + + free(current_mode.mode), + current_mode.mode = 0; +} + /* charging fallback handler */ static gboolean charging_fallback(gpointer data) { @@ -684,65 +699,35 @@ static gboolean charging_fallback(gpointer data) return(FALSE); } -static void handle_exit(void) -{ - /* exiting and clean-up when mainloop ended */ - - /* Stop appsync processes that have been started by usb-moded */ - appsync_stop(FALSE); - - hwal_cleanup(); - usb_moded_dbus_cleanup(); -#ifdef MEEGOLOCK - stop_devicelock_listener(); -#endif /* MEEGOLOCK */ - - free_mode_list(modelist); - usb_moded_module_ctx_cleanup(); - -#ifdef APP_SYNC - free_appsync_list(); -#ifdef APP_SYNC_DBUS - usb_moded_appsync_cleanup(); -#endif /* APP_SYNC_DBUS */ -#endif /* APP_SYNC */ - /* dbus_shutdown(); This causes exit(1) and don't seem - to behave as documented */ - - /* If the mainloop is initialised, unreference it */ - if (mainloop != NULL) - { - g_main_loop_quit(mainloop); - g_main_loop_unref(mainloop); - } - free(current_mode.mode); - free(current_mode.module); - - log_debug("All resources freed. Exiting!\n"); - - exit(0); -} - static void sigint_handler(int signum) { - struct mode_list_elem *data; + log_debug("handle signal: %s\n", strsignal(signum)); - if(signum == SIGINT || signum == SIGTERM) - handle_exit(); - if(signum == SIGHUP) - { - /* clean up current mode */ - data = get_usb_mode_data(); - set_disconnected_silent(data); - /* clear existing data to be sure */ - set_usb_mode_data(NULL); - /* free and read in modelist again */ - free_mode_list(modelist); + if( signum == SIGTERM ) + { + /* Assume: Stopped by init process */ + usb_moded_stop(EXIT_SUCCESS); + } + else if( signum == SIGHUP ) + { + struct mode_list_elem *data; + + /* clean up current mode */ + data = get_usb_mode_data(); + set_disconnected_silent(data); + /* clear existing data to be sure */ + set_usb_mode_data(NULL); + /* free and read in modelist again */ + free_mode_list(modelist); - modelist = read_mode_list(diag_mode); + modelist = read_mode_list(diag_mode); send_supported_modes_signal(); - } + } + else + { + usb_moded_stop(EXIT_FAILURE); + } } /* Display usage information */ @@ -822,7 +807,6 @@ static gboolean sigpipe_read_signal_cb(GIOChannel *channel, abort(); /* handle the signal */ - log_warning("handle signal: %s\n", strsignal(sig)); sigint_handler(sig); keep_watch = TRUE; @@ -1081,10 +1065,29 @@ static bool init_done_p(void) return access("/run/systemd/boot-status/init-done", F_OK) == 0; } +/** Request orderly exit from mainloop + */ +void usb_moded_stop(int exitcode) +{ + /* In case multiple exit request get done, retain the + * highest exit code used. */ + if( usb_moded_exitcode < exitcode ) + usb_moded_exitcode = exitcode; + + /* If there is no mainloop to exit, terminate immediately */ + if( !usb_moded_mainloop ) + { + log_warning("exit requested outside mainloop; exit(%d) now", + usb_moded_exitcode); + exit(usb_moded_exitcode); + } + + log_debug("stopping usb-moded mainloop"); + g_main_loop_quit(usb_moded_mainloop); +} int main(int argc, char* argv[]) { - int result = EXIT_FAILURE; int opt = 0, opt_idx = 0; struct option const options[] = { @@ -1106,6 +1109,10 @@ int main(int argc, char* argv[]) log_init(); log_name = basename(*argv); + /* - - - - - - - - - - - - - - - - - - - * + * OPTIONS + * - - - - - - - - - - - - - - - - - - - */ + /* Parse the command-line options */ while ((opt = getopt_long(argc, argv, "aifsTDdhrnvm:", options, &opt_idx)) != -1) { @@ -1165,6 +1172,10 @@ int main(int argc, char* argv[]) fprintf(stderr, "usb_moded %s starting\n", VERSION); fflush(stderr); + /* - - - - - - - - - - - - - - - - - - - * + * INITIALIZE + * - - - - - - - - - - - - - - - - - - - */ + /* silence system() calls */ if(log_type != LOG_TO_STDERR || log_level != LOG_DEBUG ) { @@ -1182,7 +1193,12 @@ int main(int argc, char* argv[]) /* Must be the 1st libdbus call that is made */ dbus_threads_init_default(); - mainloop = g_main_loop_new(NULL, FALSE); + /* signal handling */ + if( !sigpipe_init() ) + { + log_crit("signal handler init failed\n"); + goto EXIT; + } if (rescue_mode && init_done_p()) { @@ -1197,58 +1213,77 @@ int main(int argc, char* argv[]) goto EXIT; } + /* Start DBus trackers that do async initialization + * so that initial method calls are on the way while + * we do initialization actions that might block. */ + + /* DSME listener maintains in-user-mode state and is relevant + * only when MEEGOLOCK configure option has been chosen. */ +#ifdef MEEGOLOCK if( !dsme_listener_start() ) { log_crit("dsme tracking could not be started"); goto EXIT; } - - if( !systemd_control_start() ) { - log_crit("systemd control could not be started"); +#endif + /* Devicelock listener maintains devicelock state and is relevant + * only when MEEGOLOCK configure option has been chosen. */ +#ifdef MEEGOLOCK + if( !start_devicelock_listener() ) { + log_crit("devicelock tracking could not be started"); goto EXIT; } +#endif - /* init daemon into a clean state first, then dbus and hw_abstraction last */ + /* Set daemon config/state data to sane state */ usb_moded_init(); - /* Set up D-Bus Service */ - if( !usb_moded_dbus_init_service() ) - { - log_crit("usb-moded dbus service init failed\n"); + /* Allos making systemd control ipc */ + if( !systemd_control_start() ) { + log_crit("systemd control could not be started"); goto EXIT; } - if( !hwal_init() ) + /* If usb-moded happens to crash, it could leave appsync processes + * running. To make sure things are in the order expected by usb-moded + * force stopping of appsync processes during usb-moded startup. + * + * The exception is: When usb-moded starts as a part of bootup. Then + * we can be relatively sure that usb-moded has not been running yet + * and therefore no appsync processes have been started and we can + * skip the blocking ipc required to stop the appsync systemd units. */ +#ifdef APP_SYNC + if( init_done_p() ) { - /* if hw_fallback is active we can live with a failed hwal_init */ - if(!hw_fallback) - { - log_crit("hwal init failed\n"); - goto EXIT; - } + log_warning("usb-moded started after init-done; " + "forcing appsync stop"); + appsync_stop(TRUE); } -#ifdef MEEGOLOCK - start_devicelock_listener(); -#endif /* MEEGOLOCK */ +#endif - /* signal handling */ - if( !sigpipe_init() ) + /* Claim D-Bus service name before proceeding with things that + * could result in dbus signals from usb-moded interfaces to + * be broadcast */ + if( !usb_moded_dbus_init_service() ) { - log_crit("signal handler init failed\n"); + log_crit("usb-moded dbus service init failed\n"); goto EXIT; } -#ifdef SYSTEMD - /* Tell systemd that we have started up */ - if( systemd_notify ) + /* Initialize udev listener. Can cause mode changes. + * + * Failing here is allowed if '--fallback' commandline option is used. */ + if( !hwal_init() && !hw_fallback ) { - log_debug("notifying systemd\n"); - sd_notify(0, "READY=1"); + log_crit("hwal init failed\n"); + goto EXIT; } -#endif /* SYSTEMD */ + /* Broadcast supported / hidden modes */ + // TODO: should this happen before hwal_init()? send_supported_modes_signal(); send_hidden_modes_signal(); + /* Act on '--fallback' commandline option */ if(hw_fallback) { log_warning("Forcing USB state to connected always. ASK mode non functional!\n"); @@ -1256,14 +1291,77 @@ int main(int argc, char* argv[]) set_usb_connected(TRUE); } + /* - - - - - - - - - - - - - - - - - - - * + * EXECUTE + * - - - - - - - - - - - - - - - - - - - */ + + /* Tell systemd that we have started up */ +#ifdef SYSTEMD + if( systemd_notify ) + { + log_debug("notifying systemd\n"); + sd_notify(0, "READY=1"); + } +#endif + /* init succesful, run main loop */ - result = EXIT_SUCCESS; - g_main_loop_run(mainloop); + usb_moded_exitcode = EXIT_SUCCESS; + usb_moded_mainloop = g_main_loop_new(NULL, FALSE); + + log_debug("enter usb-moded mainloop"); + g_main_loop_run(usb_moded_mainloop); + log_debug("leave usb-moded mainloop"); + + g_main_loop_unref(usb_moded_mainloop), + usb_moded_mainloop = 0; + + /* - - - - - - - - - - - - - - - - - - - * + * CLEANUP + * - - - - - - - - - - - - - - - - - - - */ EXIT: - dsme_listener_stop(); - handle_exit(); + /* Detach from SystemBus. Components that hold reference to the + * shared bus connection can still perform cleanup tasks, but new + * references can't be obtained anymore and usb-moded method call + * processing no longer occurs. */ + usb_moded_dbus_cleanup(); + + /* Stop appsync processes that have been started by usb-moded */ +#ifdef APP_SYNC + appsync_stop(FALSE); +#endif + + /* Deny making systemd control ipc */ systemd_control_stop(); + /* Stop tracking devicelock status */ +#ifdef MEEGOLOCK + stop_devicelock_listener(); +#endif + /* Stop tracking device state */ +#ifdef MEEGOLOCK + dsme_listener_stop(); +#endif + + /* Stop udev listener */ + hwal_cleanup(); + + /* Release dynamically allocated config/state data */ + usb_moded_cleanup(); + + /* Detach from SessionBus connection used for APP_SYNC_DBUS. + * + * Can be handled separately from SystemBus side wind down. */ +#ifdef APP_SYNC +# ifdef APP_SYNC_DBUS + usb_moded_appsync_cleanup(); +# endif +#endif + + /* Must be done just before exit to make sure no more wakelocks + * are taken and left behind on exit path */ allow_suspend(); - return result; + + log_debug("usb-moded return from main, with exit code %d", + usb_moded_exitcode); + return usb_moded_exitcode; } diff --git a/src/usb_moded.h b/src/usb_moded.h index f965993..ed7be40 100644 --- a/src/usb_moded.h +++ b/src/usb_moded.h @@ -1,9 +1,10 @@ /* Copyright (C) 2010 Nokia Corporation. All rights reserved. - Copyright (C) 2012 Jolla. All rights reserved. + Copyright (C) 2012-2016 Jolla. All rights reserved. @author: Philippe De Swert @author: Philippe De Swert + @author: Simo Piiroinen This program is free software; you can redistribute it and/or modify it under the terms of the Lesser GNU General Public License @@ -92,4 +93,6 @@ void delay_suspend(void); extern int cable_connection_delay; +void usb_moded_stop(int exitcode); + #endif /* USB_MODED_H */