From 2ed26ed065a39df35d25407d2c66f0b05cf874b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 17 Jul 2018 16:00:21 +0200 Subject: [PATCH 1/5] execute: use structure initialization when filling in exec status --- src/core/execute.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index a08e3105bf..ed735a7660 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -4342,18 +4342,22 @@ void exec_context_free_log_extra_fields(ExecContext *c) { void exec_status_start(ExecStatus *s, pid_t pid) { assert(s); - zero(*s); - s->pid = pid; + *s = (ExecStatus) { + .pid = pid, + }; + dual_timestamp_get(&s->start_timestamp); } void exec_status_exit(ExecStatus *s, const ExecContext *context, pid_t pid, int code, int status) { assert(s); - if (s->pid && s->pid != pid) - zero(*s); + if (s->pid != pid) { + *s = (ExecStatus) { + .pid = pid, + }; + } - s->pid = pid; dual_timestamp_get(&s->exit_timestamp); s->code = code; @@ -4361,7 +4365,7 @@ void exec_status_exit(ExecStatus *s, const ExecContext *context, pid_t pid, int if (context) { if (context->utmp_id) - utmp_put_dead_process(context->utmp_id, pid, code, status); + (void) utmp_put_dead_process(context->utmp_id, pid, code, status); exec_context_tty_reset(context, NULL); } From ee39ca20c6da8b77a647e45bbc7d6ec0b852888f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 17 Jul 2018 18:47:32 +0200 Subject: [PATCH 2/5] core: drop "argv" field from ExecParameter structure We always initialize it from the same field in ExecCommand anyway, hence there's no point in passing it separately to exec_spawn(), after all we already pass the ExecCommand structure itself anyway. No change in behaviour. --- src/core/execute.c | 15 ++++----------- src/core/execute.h | 1 - src/core/service.c | 1 - src/core/socket.c | 2 -- 4 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index ed735a7660..80736abbfd 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2721,7 +2721,6 @@ static int exec_child( const ExecParameters *params, ExecRuntime *runtime, DynamicCreds *dcreds, - char **argv, int socket_fd, int named_iofds[3], int *fds, @@ -2817,7 +2816,7 @@ static int exec_child( const char *vc = params->confirm_spawn; _cleanup_free_ char *cmdline = NULL; - cmdline = exec_command_line(argv); + cmdline = exec_command_line(command->argv); if (!cmdline) { *exit_status = EXIT_MEMORY; return log_oom(); @@ -3396,7 +3395,7 @@ static int exec_child( strv_free_and_replace(accum_env, ee); } - final_argv = replace_env_argv(argv, accum_env); + final_argv = replace_env_argv(command->argv, accum_env); if (!final_argv) { *exit_status = EXIT_MEMORY; return log_oom(); @@ -3442,13 +3441,10 @@ int exec_spawn(Unit *unit, DynamicCreds *dcreds, pid_t *ret) { + int socket_fd, r, named_iofds[3] = { -1, -1, -1 }, *fds = NULL; _cleanup_strv_free_ char **files_env = NULL; - int *fds = NULL; size_t n_storage_fds = 0, n_socket_fds = 0; _cleanup_free_ char *line = NULL; - int socket_fd, r; - int named_iofds[3] = { -1, -1, -1 }; - char **argv; pid_t pid; assert(unit); @@ -3488,8 +3484,7 @@ int exec_spawn(Unit *unit, if (r < 0) return log_unit_error_errno(unit, r, "Failed to load environment files: %m"); - argv = params->argv ?: command->argv; - line = exec_command_line(argv); + line = exec_command_line(command->argv); if (!line) return log_oom(); @@ -3512,7 +3507,6 @@ int exec_spawn(Unit *unit, params, runtime, dcreds, - argv, socket_fd, named_iofds, fds, @@ -3662,7 +3656,6 @@ static void exec_command_done(ExecCommand *c) { assert(c); c->path = mfree(c->path); - c->argv = strv_free(c->argv); } diff --git a/src/core/execute.h b/src/core/execute.h index de44085492..81a118c5cd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -292,7 +292,6 @@ typedef enum ExecFlags { } ExecFlags; struct ExecParameters { - char **argv; char **environment; int *fds; diff --git a/src/core/service.c b/src/core/service.c index db1356c417..872bb77526 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1446,7 +1446,6 @@ static int service_spawn( SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS, MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE)); - exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; exec_params.fd_names = fd_names; diff --git a/src/core/socket.c b/src/core/socket.c index 56d32225c4..1bd32cf250 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1889,8 +1889,6 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { unit_set_exec_params(UNIT(s), &exec_params); - exec_params.argv = c->argv; - r = exec_spawn(UNIT(s), c, &s->exec_context, From 42cb05d5ff71e2126a35bfbc55376bda231cfc38 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 17 Jul 2018 18:50:01 +0200 Subject: [PATCH 3/5] execute: document what the different structures are for in comments --- src/core/execute.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/core/execute.h b/src/core/execute.h index 81a118c5cd..bc832eebf0 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -77,10 +77,11 @@ typedef enum ExecKeyringMode { _EXEC_KEYRING_MODE_INVALID = -1, } ExecKeyringMode; +/* Contains start and exit information about an executed command. */ struct ExecStatus { + pid_t pid; dual_timestamp start_timestamp; dual_timestamp exit_timestamp; - pid_t pid; int code; /* as in siginfo_t::si_code */ int status; /* as in sigingo_t::si_status */ }; @@ -92,6 +93,7 @@ typedef enum ExecCommandFlags { EXEC_COMMAND_AMBIENT_MAGIC = 8, } ExecCommandFlags; +/* Stores information about commands we execute. Covers both configuration settings as well as runtime data. */ struct ExecCommand { char *path; char **argv; @@ -100,13 +102,16 @@ struct ExecCommand { LIST_FIELDS(ExecCommand, command); /* useful for chaining commands */ }; +/* Encapsulates certain aspects of the runtime environment that is to be shared between multiple otherwise separate + * invocations of commands. Specifically, this allows sharing of /tmp and /var/tmp data as well as network namespaces + * between invocations of commands. This is a reference counted object, with one reference taken by each currently + * active command invocation that wants to share this runtime. */ struct ExecRuntime { int n_ref; Manager *manager; - /* unit id of the owner */ - char *id; + char *id; /* Unit id of the owner */ char *tmp_dir; char *var_tmp_dir; @@ -131,6 +136,9 @@ typedef struct ExecDirectory { mode_t mode; } ExecDirectory; +/* Encodes configuration parameters applied to invoked commands. Does not carry runtime data, but only configuration + * changes sourced from unit files and suchlike. ExecContext objects are usually embedded into Unit objects, and do not + * change after being loaded. */ struct ExecContext { char **environment; char **environment_files; @@ -291,6 +299,8 @@ typedef enum ExecFlags { EXEC_SET_WATCHDOG = 1 << 11, } ExecFlags; +/* Parameters for a specific invocation of a command. This structure is put together right before a command is + * executed. */ struct ExecParameters { char **environment; From 6a1d4d9fa6b9df1069b15f00447b3a8f9b9a3266 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 17 Jul 2018 19:36:46 +0200 Subject: [PATCH 4/5] core: properly reset all ExecStatus structures when entering a new unit cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whenever a unit is started fresh we should flush out any runtime data from the previous cycle. We are pretty good at that already, but what so far we missed was the ExecStart=/ExecStop=/… command exit status data. Let's fix that, and properly flush out that stuff too. Consider this service: [Service] ExecStart=/bin/sleep infinity ExecStop=/bin/false When this service is started, then stopped and then started again "systemctl status" would show the ExecStop= results of the previous run along with the ExecStart= results of the current one, which is very confusing. With this patch this is corrected: the data is kept right until the moment the new service cycle starts, and then flushed out. Hence "systemctl status" in that case will only show the ExecStart= data, but no ExecStop= data, like it should be. This should fix part of the confusion of #9588 --- src/core/execute.c | 24 ++++++++++++++++++++++++ src/core/execute.h | 5 +++-- src/core/mount.c | 1 + src/core/service.c | 8 +++++--- src/core/socket.c | 1 + src/core/swap.c | 1 + 6 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 80736abbfd..ccfe3e097d 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3685,6 +3685,24 @@ void exec_command_free_array(ExecCommand **c, size_t n) { c[i] = exec_command_free_list(c[i]); } +void exec_command_reset_status_array(ExecCommand *c, size_t n) { + size_t i; + + for (i = 0; i < n; i++) + exec_status_reset(&c[i].exec_status); +} + +void exec_command_reset_status_list_array(ExecCommand **c, size_t n) { + size_t i; + + for (i = 0; i < n; i++) { + ExecCommand *z; + + LIST_FOREACH(command, z, c[i]) + exec_status_reset(&z->exec_status); + } +} + typedef struct InvalidEnvInfo { const Unit *unit; const char *path; @@ -4364,6 +4382,12 @@ void exec_status_exit(ExecStatus *s, const ExecContext *context, pid_t pid, int } } +void exec_status_reset(ExecStatus *s) { + assert(s); + + *s = (ExecStatus) {}; +} + void exec_status_dump(const ExecStatus *s, FILE *f, const char *prefix) { char buf[FORMAT_TIMESTAMP_MAX]; diff --git a/src/core/execute.h b/src/core/execute.h index bc832eebf0..678ee5b189 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -340,10 +340,10 @@ int exec_spawn(Unit *unit, pid_t *ret); void exec_command_done_array(ExecCommand *c, size_t n); - ExecCommand* exec_command_free_list(ExecCommand *c); void exec_command_free_array(ExecCommand **c, size_t n); - +void exec_command_reset_status_array(ExecCommand *c, size_t n); +void exec_command_reset_status_list_array(ExecCommand **c, size_t n); void exec_command_dump_list(ExecCommand *c, FILE *f, const char *prefix); void exec_command_append_list(ExecCommand **l, ExecCommand *e); int exec_command_set(ExecCommand *c, const char *path, ...); @@ -367,6 +367,7 @@ void exec_context_free_log_extra_fields(ExecContext *c); void exec_status_start(ExecStatus *s, pid_t pid); void exec_status_exit(ExecStatus *s, const ExecContext *context, pid_t pid, int code, int status); void exec_status_dump(const ExecStatus *s, FILE *f, const char *prefix); +void exec_status_reset(ExecStatus *s); int exec_runtime_acquire(Manager *m, const ExecContext *c, const char *name, bool create, ExecRuntime **ret); ExecRuntime *exec_runtime_unref(ExecRuntime *r, bool destroy); diff --git a/src/core/mount.c b/src/core/mount.c index 21437dad08..75e1b6ac53 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1074,6 +1074,7 @@ static int mount_start(Unit *u) { m->result = MOUNT_SUCCESS; m->reload_result = MOUNT_SUCCESS; + exec_command_reset_status_array(m->exec_command, _MOUNT_EXEC_COMMAND_MAX); u->reset_accounting = true; diff --git a/src/core/service.c b/src/core/service.c index 872bb77526..7476ee5333 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1673,7 +1673,6 @@ static void service_enter_stop_post(Service *s, ServiceResult f) { s->result = f; service_unwatch_control_pid(s); - (void) unit_enqueue_rewatch_pids(UNIT(s)); s->control_command = s->exec_command[SERVICE_EXEC_STOP_POST]; @@ -2252,8 +2251,6 @@ static int service_start(Unit *u) { s->main_pid_alien = false; s->forbid_restart = false; - u->reset_accounting = true; - s->status_text = mfree(s->status_text); s->status_errno = 0; @@ -2262,12 +2259,17 @@ static int service_start(Unit *u) { s->watchdog_override_enable = false; s->watchdog_override_usec = 0; + exec_command_reset_status_list_array(s->exec_command, _SERVICE_EXEC_COMMAND_MAX); + exec_status_reset(&s->main_exec_status); + /* This is not an automatic restart? Flush the restart counter then */ if (s->flush_n_restarts) { s->n_restarts = 0; s->flush_n_restarts = false; } + u->reset_accounting = true; + service_enter_start_pre(s); return 1; } diff --git a/src/core/socket.c b/src/core/socket.c index 1bd32cf250..a6127654b5 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2457,6 +2457,7 @@ static int socket_start(Unit *u) { return r; s->result = SOCKET_SUCCESS; + exec_command_reset_status_list_array(s->exec_command, _SOCKET_EXEC_COMMAND_MAX); u->reset_accounting = true; diff --git a/src/core/swap.c b/src/core/swap.c index b78b1aa266..f8d6a4dd22 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -853,6 +853,7 @@ static int swap_start(Unit *u) { return r; s->result = SWAP_SUCCESS; + exec_command_reset_status_array(s->exec_command, _SWAP_EXEC_COMMAND_MAX); u->reset_accounting = true; From 3e14d36a8a0eedbdd6de4b7944ad86696e78fadd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 17 Jul 2018 19:41:13 +0200 Subject: [PATCH 5/5] systemctl: show ExecStop= data after ExecStart= data the service manager serializes ExecStop= execution data after ExecStart=, like it makes sense and how it should be expected. However, systemctl previously would reverse them when deserializing them locally, and thus show ExecStop= results before ExecStart= results. And that's confusing. Let's fix that. --- src/systemctl/systemctl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index d9bef997d5..93c1979d19 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -4548,6 +4548,7 @@ static int map_asserts(sd_bus *bus, const char *member, sd_bus_message *m, sd_bu static int map_exec(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) { _cleanup_free_ ExecStatusInfo *info = NULL; + ExecStatusInfo *last; UnitStatusInfo *i = userdata; int r; @@ -4559,13 +4560,16 @@ static int map_exec(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_e if (!info) return -ENOMEM; + LIST_FIND_TAIL(exec, i->exec, last); + while ((r = exec_status_info_deserialize(m, info)) > 0) { info->name = strdup(member); if (!info->name) return -ENOMEM; - LIST_PREPEND(exec, i->exec, info); + LIST_INSERT_AFTER(exec, i->exec, last, info); + last = info; info = new0(ExecStatusInfo, 1); if (!info)