From 4b6a2b3f584bd06f6b0df450ec9d9d7ee537249c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 17 Oct 2022 09:01:12 +0200 Subject: [PATCH 1/3] core: modernize style of return param naming --- src/core/manager.c | 50 +++++++++++++++++++++------------------------- src/core/manager.h | 4 ++-- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 22c79bed17..9838789435 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2116,15 +2116,13 @@ int manager_load_unit_prepare( const char *name, const char *path, sd_bus_error *e, - Unit **_ret) { + Unit **ret) { - _cleanup_(unit_freep) Unit *cleanup_ret = NULL; - Unit *ret; - UnitType t; + _cleanup_(unit_freep) Unit *cleanup_unit = NULL; int r; assert(m); - assert(_ret); + assert(ret); /* This will prepare the unit for loading, but not actually load anything from disk. */ @@ -2139,7 +2137,7 @@ int manager_load_unit_prepare( name = basename(path); } - t = unit_name_to_type(name); + UnitType t = unit_name_to_type(name); if (t == _UNIT_TYPE_INVALID || !unit_name_is_valid(name, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE)) { if (unit_name_is_valid(name, UNIT_NAME_TEMPLATE)) @@ -2148,8 +2146,8 @@ int manager_load_unit_prepare( return sd_bus_error_setf(e, SD_BUS_ERROR_INVALID_ARGS, "Unit name %s is not valid.", name); } - ret = manager_get_unit(m, name); - if (ret) { + Unit *unit = manager_get_unit(m, name); + if (unit) { /* The time-based cache allows to start new units without daemon-reload, * but if they are already referenced (because of dependencies or ordering) * then we have to force a load of the fragment. As an optimization, check @@ -2159,34 +2157,34 @@ int manager_load_unit_prepare( * we need to try again — even if the cache is current, it might have been * updated in a different context before we had a chance to retry loading * this particular unit. */ - if (manager_unit_cache_should_retry_load(ret)) - ret->load_state = UNIT_STUB; + if (manager_unit_cache_should_retry_load(unit)) + unit->load_state = UNIT_STUB; else { - *_ret = ret; + *ret = unit; return 1; } } else { - ret = cleanup_ret = unit_new(m, unit_vtable[t]->object_size); - if (!ret) + unit = cleanup_unit = unit_new(m, unit_vtable[t]->object_size); + if (!unit) return -ENOMEM; } if (path) { - r = free_and_strdup(&ret->fragment_path, path); + r = free_and_strdup(&unit->fragment_path, path); if (r < 0) return r; } - r = unit_add_name(ret, name); + r = unit_add_name(unit, name); if (r < 0) return r; - unit_add_to_load_queue(ret); - unit_add_to_dbus_queue(ret); - unit_add_to_gc_queue(ret); + unit_add_to_load_queue(unit); + unit_add_to_dbus_queue(unit); + unit_add_to_gc_queue(unit); - *_ret = ret; - cleanup_ret = NULL; + *ret = unit; + TAKE_PTR(cleanup_unit); return 0; } @@ -2196,23 +2194,21 @@ int manager_load_unit( const char *name, const char *path, sd_bus_error *e, - Unit **_ret) { - + Unit **ret) { int r; assert(m); - assert(_ret); + assert(ret); - /* This will load the service information files, but not actually - * start any services or anything. */ + /* This will load the unit config, but not actually start any services or anything. */ - r = manager_load_unit_prepare(m, name, path, e, _ret); + r = manager_load_unit_prepare(m, name, path, e, ret); if (r != 0) return r; manager_dispatch_load_queue(m); - *_ret = unit_follow_merge(*_ret); + *ret = unit_follow_merge(*ret); return 0; } diff --git a/src/core/manager.h b/src/core/manager.h index ab4b30cfe7..75c16d6e26 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -494,8 +494,8 @@ Unit *manager_get_unit(Manager *m, const char *name); int manager_get_job_from_dbus_path(Manager *m, const char *s, Job **_j); bool manager_unit_cache_should_retry_load(Unit *u); -int manager_load_unit_prepare(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **_ret); -int manager_load_unit(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **_ret); +int manager_load_unit_prepare(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **ret); +int manager_load_unit(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **ret); int manager_load_startable_unit_or_warn(Manager *m, const char *name, const char *path, Unit **ret); int manager_load_unit_from_dbus_path(Manager *m, const char *s, sd_bus_error *e, Unit **_u); From 535b7fcb44f0a862e1707d0cb788afbbd20ee31e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 17 Oct 2022 10:48:52 +0200 Subject: [PATCH 2/3] core: simplify the return convention in manager_load_unit() This function was returning 0 or 1 on success. It has many callers, and it wasn't clear if any of them care about the distinction. It turns out they don't and the return values were done for convenience because manager_load_unit_prepare() returns 0 or 1. Let's invert the code in the static function to follow the usual pattern where 0 means "no work was done" and 1 means "work was done", and make the non-static function always return 0 to make the code easier to read, and also add comments that explain what is happening. No functional change. --- src/core/manager.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 9838789435..a59afafb58 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2161,7 +2161,7 @@ int manager_load_unit_prepare( unit->load_state = UNIT_STUB; else { *ret = unit; - return 1; + return 0; /* The unit was already loaded */ } } else { unit = cleanup_unit = unit_new(m, unit_vtable[t]->object_size); @@ -2186,7 +2186,7 @@ int manager_load_unit_prepare( *ret = unit; TAKE_PTR(cleanup_unit); - return 0; + return 1; /* The unit was added the load queue */ } int manager_load_unit( @@ -2203,11 +2203,11 @@ int manager_load_unit( /* This will load the unit config, but not actually start any services or anything. */ r = manager_load_unit_prepare(m, name, path, e, ret); - if (r != 0) + if (r <= 0) return r; + /* Unit was newly loaded */ manager_dispatch_load_queue(m); - *ret = unit_follow_merge(*ret); return 0; } From e2d4456f43d48b0fefe66524bdcdae0e143d6b46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 17 Oct 2022 11:31:52 +0200 Subject: [PATCH 3/3] man: fix method name --- man/org.freedesktop.systemd1.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 9fb215a136..de4b13c84b 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -2442,7 +2442,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { will be a pair of empty strings. Transient contains a boolean that indicates whether the unit was created as a - transient unit (i.e. via CreateTransientUnit() on the manager object). + transient unit (i.e. via StartTransientUnit() on the manager object). ActivationDetails contains a list of string pairs, key and value, that describe the event that caused the unit to be activated, if any. The key describes the information