From f6aac5bf1bca20536a2b51bf9bce1d8e8208be86 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 13:35:52 +0100 Subject: [PATCH 1/8] socket-util: clarify why sockaddr_port returns unsigned rather than uint16_t --- src/basic/socket-util.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index fa74465b92..b765fb6125 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -539,22 +539,25 @@ bool socket_address_matches_fd(const SocketAddress *a, int fd) { return socket_address_equal(a, &b); } -int sockaddr_port(const struct sockaddr *_sa, unsigned *port) { +int sockaddr_port(const struct sockaddr *_sa, unsigned *ret_port) { union sockaddr_union *sa = (union sockaddr_union*) _sa; + /* Note, this returns the port as 'unsigned' rather than 'uint16_t', as AF_VSOCK knows larger ports */ + assert(sa); switch (sa->sa.sa_family) { + case AF_INET: - *port = be16toh(sa->in.sin_port); + *ret_port = be16toh(sa->in.sin_port); return 0; case AF_INET6: - *port = be16toh(sa->in6.sin6_port); + *ret_port = be16toh(sa->in6.sin6_port); return 0; case AF_VSOCK: - *port = sa->vm.svm_port; + *ret_port = sa->vm.svm_port; return 0; default: From dfde7e8c5bd1a7846d9526b63c80d111dbd0c5e7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 13:36:07 +0100 Subject: [PATCH 2/8] sd-daemon: use sockaddr_port() helper --- src/libsystemd/sd-daemon/sd-daemon.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index 64a74929bf..ba383e0b3b 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -306,17 +306,13 @@ _public_ int sd_is_socket_inet(int fd, int family, int type, int listening, uint return 0; if (port > 0) { - if (sockaddr.sa.sa_family == AF_INET) { - if (l < sizeof(struct sockaddr_in)) - return -EINVAL; + unsigned sa_port; - return htobe16(port) == sockaddr.in.sin_port; - } else { - if (l < sizeof(struct sockaddr_in6)) - return -EINVAL; + r = sockaddr_port(&sockaddr.sa, &sa_port); + if (r < 0) + return r; - return htobe16(port) == sockaddr.in6.sin6_port; - } + return port == sa_port; } return 1; From a6bcef29579409872735a2cfbf77d1c61ea91332 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 15:58:06 +0100 Subject: [PATCH 3/8] analyze: port verb dispatching to verbs.[ch] API Let's unify the code for parsing command line verbs, and reuse the common verbs.[ch] API in systemd-analyze too. This adds a couple of error messages when people pass too many arguments. Moreover thus pushes bus allocation into the verb functions, which corrects a couple of cases where we previously allocated a bus but really didn't need to. Other than that behaviour shouldn't really change. --- src/analyze/analyze.c | 213 +++++++++++++++++++++--------------------- 1 file changed, 108 insertions(+), 105 deletions(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 7f35b04c31..9efef5d7ec 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -47,6 +47,7 @@ #include "terminal-util.h" #include "unit-name.h" #include "util.h" +#include "verbs.h" #define SCALE_X (0.1 / 1000.0) /* pixels per us */ #define SCALE_Y (20.0) @@ -582,15 +583,20 @@ static void svg_graph_box(double height, double begin, double end) { } } -static int analyze_plot(sd_bus *bus) { +static int analyze_plot(int argc, char *argv[], void *userdata) { _cleanup_(free_host_infop) struct host_info *host = NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; struct unit_times *times; struct boot_times *boot; - int n, m = 1, y=0; + int n, m = 1, y = 0, r; double width; _cleanup_free_ char *pretty_times = NULL; struct unit_times *u; + r = acquire_bus(true, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); + n = acquire_boot_times(bus, &boot); if (n < 0) return n; @@ -984,12 +990,17 @@ static int list_dependencies(sd_bus *bus, const char *name) { return list_dependencies_one(bus, name, 0, &units, 0); } -static int analyze_critical_chain(sd_bus *bus, char *names[]) { +static int analyze_critical_chain(int argc, char *argv[], void *userdata) { + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; struct unit_times *times; unsigned int i; Hashmap *h; int n, r; + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); + n = acquire_time_data(bus, ×); if (n <= 0) return n; @@ -1010,22 +1021,27 @@ static int analyze_critical_chain(sd_bus *bus, char *names[]) { puts("The time after the unit is active or started is printed after the \"@\" character.\n" "The time the unit takes to start is printed after the \"+\" character.\n"); - if (!strv_isempty(names)) { + if (argc > 1) { char **name; - STRV_FOREACH(name, names) + STRV_FOREACH(name, strv_skip(argv, 1)) list_dependencies(bus, *name); } else list_dependencies(bus, SPECIAL_DEFAULT_TARGET); - hashmap_free(h); + h = hashmap_free(h); free_unit_times(times, (unsigned) n); return 0; } -static int analyze_blame(sd_bus *bus) { +static int analyze_blame(int argc, char *argv[], void *userdata) { + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; struct unit_times *times; unsigned i; - int n; + int n, r; + + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); n = acquire_time_data(bus, ×); if (n <= 0) @@ -1046,10 +1062,15 @@ static int analyze_blame(sd_bus *bus) { return 0; } -static int analyze_time(sd_bus *bus) { +static int analyze_time(int argc, char *argv[], void *userdata) { + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *buf = NULL; int r; + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); + r = pretty_boot_time(bus, &buf); if (r < 0) return r; @@ -1170,16 +1191,21 @@ static int expand_patterns(sd_bus *bus, char **patterns, char ***ret) { return 0; } -static int dot(sd_bus *bus, char* patterns[]) { +static int dot(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_strv_free_ char **expanded_patterns = NULL; _cleanup_strv_free_ char **expanded_from_patterns = NULL; _cleanup_strv_free_ char **expanded_to_patterns = NULL; int r; UnitInfo u; - r = expand_patterns(bus, patterns, &expanded_patterns); + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); + + r = expand_patterns(bus, strv_skip(argv, 1), &expanded_patterns); if (r < 0) return r; @@ -1235,16 +1261,16 @@ static int dot(sd_bus *bus, char* patterns[]) { return 0; } -static int dump(sd_bus *bus, char **args) { - _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; +static int dump(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; const char *text = NULL; int r; - if (!strv_isempty(args)) { - log_error("Too many arguments."); - return -E2BIG; - } + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); pager_open(arg_no_pager, false); @@ -1268,17 +1294,17 @@ static int dump(sd_bus *bus, char **args) { return 0; } -static int set_log_level(sd_bus *bus, char **args) { +static int set_log_level(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; int r; - assert(bus); - assert(args); + assert(argc == 2); + assert(argv); - if (strv_length(args) != 1) { - log_error("This command expects one argument only."); - return -E2BIG; - } + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); r = sd_bus_set_property( bus, @@ -1288,25 +1314,22 @@ static int set_log_level(sd_bus *bus, char **args) { "LogLevel", &error, "s", - args[0]); + argv[1]); if (r < 0) return log_error_errno(r, "Failed to issue method call: %s", bus_error_message(&error, r)); return 0; } -static int get_log_level(sd_bus *bus, char **args) { +static int get_log_level(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - int r; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *level = NULL; + int r; - assert(bus); - assert(args); - - if (!strv_isempty(args)) { - log_error("Too many arguments."); - return -E2BIG; - } + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); r = sd_bus_get_property_string( bus, @@ -1323,17 +1346,17 @@ static int get_log_level(sd_bus *bus, char **args) { return 0; } -static int set_log_target(sd_bus *bus, char **args) { +static int set_log_target(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; int r; - assert(bus); - assert(args); + assert(argc == 2); + assert(argv); - if (strv_length(args) != 1) { - log_error("This command expects one argument only."); - return -E2BIG; - } + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); r = sd_bus_set_property( bus, @@ -1343,25 +1366,22 @@ static int set_log_target(sd_bus *bus, char **args) { "LogTarget", &error, "s", - args[0]); + argv[1]); if (r < 0) return log_error_errno(r, "Failed to issue method call: %s", bus_error_message(&error, r)); return 0; } -static int get_log_target(sd_bus *bus, char **args) { +static int get_log_target(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - int r; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *target = NULL; + int r; - assert(bus); - assert(args); - - if (!strv_isempty(args)) { - log_error("Too many arguments."); - return -E2BIG; - } + r = acquire_bus(false, &bus); + if (r < 0) + return log_error_errno(r, "Failed to create bus connection: %m"); r = sd_bus_get_property_string( bus, @@ -1388,12 +1408,12 @@ static void dump_syscall_filter(const SyscallFilterSet *set) { printf(" %s\n", syscall); } -static int dump_syscall_filters(char** names) { +static int dump_syscall_filters(int argc, char *argv[], void *userdata) { bool first = true; pager_open(arg_no_pager, false); - if (strv_isempty(names)) { + if (strv_isempty(strv_skip(argv, 1))) { int i; for (i = 0; i < _SYSCALL_FILTER_SET_MAX; i++) { @@ -1405,7 +1425,7 @@ static int dump_syscall_filters(char** names) { } else { char **name; - STRV_FOREACH(name, names) { + STRV_FOREACH(name, strv_skip(argv, 1)) { const SyscallFilterSet *set; if (!first) @@ -1435,19 +1455,14 @@ static int dump_syscall_filters(char** names) { } #endif -static int test_calendar(char **args) { +static int test_calendar(int argc, char *argv[], void *userdata) { int ret = 0, r; char **p; usec_t n; - if (strv_isempty(args)) { - log_error("Expected at least one calendar specification string as argument."); - return -EINVAL; - } - n = now(CLOCK_REALTIME); - STRV_FOREACH(p, args) { + STRV_FOREACH(p, strv_skip(argv, 1)) { _cleanup_(calendar_spec_freep) CalendarSpec *spec = NULL; _cleanup_free_ char *t = NULL; usec_t next; @@ -1499,7 +1514,14 @@ static int test_calendar(char **args) { return ret; } -static void help(void) { +static int do_verify(int argc, char *argv[], void *userdata) { + return verify_units(strv_skip(argv, 1), + arg_user ? UNIT_FILE_USER : UNIT_FILE_SYSTEM, + arg_man, + arg_generators); +} + +static int help(int argc, char *argv[], void *userdata) { pager_open(arg_no_pager, false); @@ -1539,6 +1561,8 @@ static void help(void) { /* When updating this list, including descriptions, apply * changes to shell-completion/bash/systemd-analyze and * shell-completion/zsh/_systemd-analyze too. */ + + return 0; } static int parse_argv(int argc, char *argv[]) { @@ -1583,8 +1607,7 @@ static int parse_argv(int argc, char *argv[]) { switch (c) { case 'h': - help(); - return 0; + return help(0, NULL, NULL); case ARG_VERSION: return version(); @@ -1676,10 +1699,30 @@ static int parse_argv(int argc, char *argv[]) { } int main(int argc, char *argv[]) { + + static const Verb verbs[] = { + { "help", VERB_ANY, VERB_ANY, 0, help }, + { "time", VERB_ANY, 1, VERB_DEFAULT, analyze_time }, + { "blame", VERB_ANY, 1, 0, analyze_blame }, + { "critical-chain", VERB_ANY, VERB_ANY, 0, analyze_critical_chain }, + { "plot", VERB_ANY, 1, 0, analyze_plot }, + { "dot", VERB_ANY, VERB_ANY, 0, dot }, + { "set-log-level", 2, 2, 0, set_log_level }, + { "get-log-level", VERB_ANY, 1, 0, get_log_level }, + { "set-log-target", 2, 2, 0, set_log_target }, + { "get-log-target", VERB_ANY, 1, 0, get_log_target }, + { "dump", VERB_ANY, 1, 0, dump }, + { "syscall-filter", VERB_ANY, VERB_ANY, 0, dump_syscall_filters }, + { "verify", 2, VERB_ANY, 0, do_verify }, + { "calendar", 2, VERB_ANY, 0, test_calendar }, + {} + }; + int r; setlocale(LC_ALL, ""); setlocale(LC_NUMERIC, "C"); /* we want to format/parse floats in C style */ + log_parse_environment(); log_open(); @@ -1687,47 +1730,7 @@ int main(int argc, char *argv[]) { if (r <= 0) goto finish; - if (streq_ptr(argv[optind], "verify")) - r = verify_units(argv+optind+1, - arg_user ? UNIT_FILE_USER : UNIT_FILE_SYSTEM, - arg_man, - arg_generators); - else { - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - - r = acquire_bus(streq_ptr(argv[optind], "plot"), &bus); - if (r < 0) { - log_error_errno(r, "Failed to create bus connection: %m"); - goto finish; - } - - if (!argv[optind] || streq(argv[optind], "time")) - r = analyze_time(bus); - else if (streq(argv[optind], "blame")) - r = analyze_blame(bus); - else if (streq(argv[optind], "critical-chain")) - r = analyze_critical_chain(bus, argv+optind+1); - else if (streq(argv[optind], "plot")) - r = analyze_plot(bus); - else if (streq(argv[optind], "dot")) - r = dot(bus, argv+optind+1); - else if (streq(argv[optind], "dump")) - r = dump(bus, argv+optind+1); - else if (streq(argv[optind], "set-log-level")) - r = set_log_level(bus, argv+optind+1); - else if (streq(argv[optind], "get-log-level")) - r = get_log_level(bus, argv+optind+1); - else if (streq(argv[optind], "set-log-target")) - r = set_log_target(bus, argv+optind+1); - else if (streq(argv[optind], "get-log-target")) - r = get_log_target(bus, argv+optind+1); - else if (streq(argv[optind], "syscall-filter")) - r = dump_syscall_filters(argv+optind+1); - else if (streq(argv[optind], "calendar")) - r = test_calendar(argv+optind+1); - else - log_error("Unknown operation '%s'.", argv[optind]); - } + r = dispatch_verb(argc, argv, verbs, NULL); finish: pager_close(); From f72b7018d2828ae617c2a6001d088c6e6cd93294 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 16:00:54 +0100 Subject: [PATCH 4/8] analyze: arg_host can be "const char*", hence make it so. --- src/analyze/analyze.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 9efef5d7ec..16eba2d90f 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -79,7 +79,7 @@ static char** arg_dot_to_patterns = NULL; static usec_t arg_fuzz = 0; static bool arg_no_pager = false; static BusTransport arg_transport = BUS_TRANSPORT_LOCAL; -static char *arg_host = NULL; +static const char *arg_host = NULL; static bool arg_user = false; static bool arg_man = true; static bool arg_generators = false; From 8efbce138ab5d43ee002bba2064118a72daaf9f4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 16:01:14 +0100 Subject: [PATCH 5/8] analyze: add some logging to some error cases --- src/analyze/analyze.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 16eba2d90f..ec2504a1c0 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1007,12 +1007,12 @@ static int analyze_critical_chain(int argc, char *argv[], void *userdata) { h = hashmap_new(&string_hash_ops); if (!h) - return -ENOMEM; + return log_oom(); - for (i = 0; i < (unsigned)n; i++) { + for (i = 0; i < (unsigned) n; i++) { r = hashmap_put(h, times[i].name, ×[i]); if (r < 0) - return r; + return log_error_errno(r, "Failed to add entry to hashmap: %m"); } unit_times_hashmap = h; From 8486c92394d7ff2050608c6f335f000f2942a21a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 16:01:43 +0100 Subject: [PATCH 6/8] analyze: fix indentation in one case --- src/analyze/analyze.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index ec2504a1c0..ae635f0368 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1276,13 +1276,13 @@ static int dump(int argc, char *argv[], void *userdata) { r = sd_bus_call_method( bus, - "org.freedesktop.systemd1", - "/org/freedesktop/systemd1", - "org.freedesktop.systemd1.Manager", - "Dump", - &error, - &reply, - ""); + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "Dump", + &error, + &reply, + NULL); if (r < 0) return log_error_errno(r, "Failed issue method call: %s", bus_error_message(&error, r)); From bc6695ec7e602f76523342d6e211dea56d092836 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 16:01:49 +0100 Subject: [PATCH 7/8] analyze: correct help text where we take unit name arguments --- src/analyze/analyze.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index ae635f0368..93bc9c0273 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1545,9 +1545,9 @@ static int help(int argc, char *argv[], void *userdata) { "Commands:\n" " time Print time spent in the kernel\n" " blame Print list of running units ordered by time to init\n" - " critical-chain Print a tree of the time critical chain of units\n" + " critical-chain [UNIT...] Print a tree of the time critical chain of units\n" " plot Output SVG graphic showing service initialization\n" - " dot Output dependency graph in man:dot(1) format\n" + " dot [UNIT...] Output dependency graph in man:dot(1) format\n" " set-log-level LEVEL Set logging threshold for manager\n" " set-log-target TARGET Set logging target for manager\n" " get-log-level Get logging threshold for manager\n" From 6c6d285fbe756b148fed56b8ae734ef40cab5760 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Dec 2017 16:02:10 +0100 Subject: [PATCH 8/8] update TODO --- TODO | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TODO b/TODO index b022b601ef..c7a76796ea 100644 --- a/TODO +++ b/TODO @@ -319,7 +319,7 @@ Features: * Rework systemctl's GetAll property parsing to use the generic bus_map_all_properties() API * Port various tools to make use of verbs.[ch], where applicable: busctl, - coredumpctl, hostnamectl, localectl, systemd-analyze, timedatectl + coredumpctl, hostnamectl, localectl, timedatectl * hostnamectl: show root image uuid