From e603692adcd50a4ea5b3043caf360dba5c38a8dd Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 3 May 2012 04:38:53 +0300 Subject: [PATCH 1/6] Make transports.c logs a bit more helpful. --- src/or/transports.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index 564603e1fe..b8b2331c5a 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -340,11 +340,12 @@ pt_configure_remaining_proxies(void) /* This proxy is marked by a SIGHUP. Check whether we need to restart it. */ if (proxy_needs_restart(mp)) { - log_info(LD_GENERAL, "Preparing managed proxy for restart."); + log_info(LD_GENERAL, "Preparing managed proxy '%s' for restart.", + mp->argv[0]); proxy_prepare_for_restart(mp); } else { /* it doesn't need to be restarted. */ - log_info(LD_GENERAL, "Nothing changed for managed proxy after HUP: " - "not restarting."); + log_info(LD_GENERAL, "Nothing changed for managed proxy '%s' after HUP: " + "not restarting.", mp->argv[0]); } continue; @@ -387,7 +388,8 @@ configure_proxy(managed_proxy_t *mp) pos = tor_read_all_handle(tor_process_get_stdout_pipe(mp->process_handle), stdout_buf, sizeof(stdout_buf) - 1, NULL); if (pos < 0) { - log_notice(LD_GENERAL, "Failed to read data from managed proxy"); + log_notice(LD_GENERAL, "Failed to read data from managed proxy '%s'.", + mp->argv[0]); mp->conf_state = PT_PROTO_BROKEN; goto done; } @@ -449,11 +451,13 @@ configure_proxy(managed_proxy_t *mp) } else if (r == IO_STREAM_EAGAIN) { /* check back later */ return; } else if (r == IO_STREAM_CLOSED || r == IO_STREAM_TERM) { /* snap! */ - log_notice(LD_GENERAL, "Managed proxy stream closed. " - "Most probably application stopped running"); + log_warn(LD_GENERAL, "Our communication channel with the managed proxy " + "'%s' closed. Most probably application stopped running.", + mp->argv[0]); mp->conf_state = PT_PROTO_BROKEN; } else { /* unknown stream status */ - log_notice(LD_GENERAL, "Unknown stream status while configuring proxy."); + log_warn(LD_BUG, "Unknown stream status '%d' while configuring managed " + "proxy '%s'.", r, mp->argv[0]); } /* if the proxy finished configuring, exit the loop. */ @@ -586,8 +590,8 @@ handle_finished_proxy(managed_proxy_t *mp) case PT_PROTO_ACCEPTING_METHODS: case PT_PROTO_COMPLETED: default: - log_warn(LD_CONFIG, "Unexpected managed proxy state in " - "handle_finished_proxy()."); + log_warn(LD_CONFIG, "Unexpected state '%d' of managed proxy '%s'.", + mp->conf_state, mp->argv[0]); tor_assert(0); } @@ -612,11 +616,13 @@ handle_methods_done(const managed_proxy_t *mp) tor_assert(mp->transports); if (smartlist_len(mp->transports) == 0) - log_notice(LD_GENERAL, "Proxy was spawned successfully, " - "but it didn't laucn any pluggable transport listeners!"); + log_notice(LD_GENERAL, "Managed proxy '%s' was spawned successfully, " + "but it didn't launch any pluggable transport listeners!", + mp->argv[0]); - log_info(LD_CONFIG, "%s managed proxy configuration completed!", - mp->is_server ? "Server" : "Client"); + log_info(LD_CONFIG, "%s managed proxy '%s' configuration completed!", + mp->is_server ? "Server" : "Client", + mp->argv[0]); } /** Handle a configuration protocol line received from a @@ -624,7 +630,8 @@ handle_methods_done(const managed_proxy_t *mp) void handle_proxy_line(const char *line, managed_proxy_t *mp) { - log_debug(LD_GENERAL, "Got a line from managed proxy: %s", line); + log_info(LD_GENERAL, "Got a line from managed proxy '%s': (%s)", + mp->argv[0], line); if (strlen(line) < SMALLEST_MANAGED_LINE_SIZE) { log_warn(LD_GENERAL, "Managed proxy configuration line is too small. " @@ -710,7 +717,7 @@ handle_proxy_line(const char *line, managed_proxy_t *mp) err: mp->conf_state = PT_PROTO_BROKEN; log_warn(LD_CONFIG, "Managed proxy at '%s' failed the configuration protocol" - " and will be destroyed.", mp->argv ? mp->argv[0] : ""); + " and will be destroyed.", mp->argv[0]); } /** Parses an ENV-ERROR line and warns the user accordingly. */ From 9ceec869b5901776285e72d659faa9ba67b80faa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 May 2012 09:55:14 -0400 Subject: [PATCH 2/6] Document some transports.c behaviors and assumptions --- src/or/transports.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index b8b2331c5a..09fd69bf92 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -1034,8 +1034,11 @@ create_managed_proxy_environment(const managed_proxy_t *mp) } /** Create and return a new managed proxy for transport using - * proxy_argv. If is_server is true, it's a server - * managed proxy. */ + * proxy_argv. Also, add it to the global managed proxy list. If + * is_server is true, it's a server managed proxy. Takes ownership of + * proxy_argv. + * + * Requires that proxy_argv have at least one element. */ static managed_proxy_t * managed_proxy_create(const smartlist_t *transport_list, char **proxy_argv, int is_server) @@ -1063,7 +1066,13 @@ managed_proxy_create(const smartlist_t *transport_list, /** Register proxy with proxy_argv, supporting transports in * transport_list, to the managed proxy subsystem. - * If is_server is true, then the proxy is a server proxy. */ + * If is_server is true, then the proxy is a server proxy. + * + * Takes ownership of proxy_argv. + * + * Requires that proxy_argv be a NULL-terminated array of command-line + * elements, containing at least one element. + **/ void pt_kickstart_proxy(const smartlist_t *transport_list, char **proxy_argv, int is_server) @@ -1071,6 +1080,10 @@ pt_kickstart_proxy(const smartlist_t *transport_list, managed_proxy_t *mp=NULL; transport_t *old_transport = NULL; + if (!proxy_argv || !proxy_argv[0]) { + return; + } + mp = get_managed_proxy_by_argv_and_type(proxy_argv, is_server); if (!mp) { /* we haven't seen this proxy before */ From a75941dcd54be0bfca8bf0709e74c959f8d2cb04 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 May 2012 09:56:12 -0400 Subject: [PATCH 3/6] Changes file for bug 5070 --- changes/bug5070 | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changes/bug5070 diff --git a/changes/bug5070 b/changes/bug5070 new file mode 100644 index 0000000000..0b8d00ad27 --- /dev/null +++ b/changes/bug5070 @@ -0,0 +1,3 @@ + o Minor features: + - Improve log messages about managed transports. Resolves ticket + 5070. From 39e69a0a8c9147f140de327b20ba44619297e738 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 May 2012 10:57:59 -0400 Subject: [PATCH 4/6] Fix comments: There is no such thing as a NUL pointer --- src/or/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index bfed4e5db4..ab4f160bf2 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -5099,7 +5099,7 @@ parse_client_transport_line(const char *line, int validate_only) *tmp++ = smartlist_get(items, 2); smartlist_del_keeporder(items, 2); } - *tmp = NULL; /*terminated with NUL pointer, just like execve() likes it*/ + *tmp = NULL; /*terminated with NULL, just like execve() likes it*/ /* kickstart the thing */ pt_kickstart_client_proxy(transport_list, proxy_argv); @@ -5222,7 +5222,7 @@ parse_server_transport_line(const char *line, int validate_only) *tmp++ = smartlist_get(items, 2); smartlist_del_keeporder(items, 2); } - *tmp = NULL; /*terminated with NUL pointer, just like execve() likes it*/ + *tmp = NULL; /*terminated with NULL, just like execve() likes it*/ /* kickstart the thing */ pt_kickstart_server_proxy(transport_list, proxy_argv); From 74810f95ade6e35ea841442dba7459d4c4ce0179 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 May 2012 10:59:23 -0400 Subject: [PATCH 5/6] Fix an overwide line --- src/or/transports.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index 09fd69bf92..eb076ae129 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -344,8 +344,8 @@ pt_configure_remaining_proxies(void) mp->argv[0]); proxy_prepare_for_restart(mp); } else { /* it doesn't need to be restarted. */ - log_info(LD_GENERAL, "Nothing changed for managed proxy '%s' after HUP: " - "not restarting.", mp->argv[0]); + log_info(LD_GENERAL, "Nothing changed for managed proxy '%s' after " + "HUP: not restarting.", mp->argv[0]); } continue; From eefdb9eec25463df012a27cdc28b6f7b73831d29 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 7 May 2012 11:02:17 -0400 Subject: [PATCH 6/6] Using %d to printf an enum may not be by-the-standard okay. --- src/or/transports.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index eb076ae129..07b9371158 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -457,7 +457,7 @@ configure_proxy(managed_proxy_t *mp) mp->conf_state = PT_PROTO_BROKEN; } else { /* unknown stream status */ log_warn(LD_BUG, "Unknown stream status '%d' while configuring managed " - "proxy '%s'.", r, mp->argv[0]); + "proxy '%s'.", (int)r, mp->argv[0]); } /* if the proxy finished configuring, exit the loop. */ @@ -591,7 +591,7 @@ handle_finished_proxy(managed_proxy_t *mp) case PT_PROTO_COMPLETED: default: log_warn(LD_CONFIG, "Unexpected state '%d' of managed proxy '%s'.", - mp->conf_state, mp->argv[0]); + (int)mp->conf_state, mp->argv[0]); tor_assert(0); }