From 601dc59be28275a0b4ac499dd0e72233d73b39d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Jan 2022 18:09:22 +0100 Subject: [PATCH 1/3] Use ASSERT_PTR() in more places --- src/core/unit-printf.c | 41 +++++++++---------------------------- src/shared/install-printf.c | 18 ++++++---------- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 46c383b8419..774be7ba6f3 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -13,28 +13,22 @@ #include "user-util.h" static int specifier_prefix_and_instance(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; - - assert(u); + const Unit *u = ASSERT_PTR(userdata); return unit_name_to_prefix_and_instance(u->id, ret); } static int specifier_prefix(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; - - assert(u); + const Unit *u = ASSERT_PTR(userdata); return unit_name_to_prefix(u->id, ret); } static int specifier_prefix_unescaped(char specifier, const void *data, const char *root, const void *userdata, char **ret) { _cleanup_free_ char *p = NULL; - const Unit *u = userdata; + const Unit *u = ASSERT_PTR(userdata); int r; - assert(u); - r = unit_name_to_prefix(u->id, &p); if (r < 0) return r; @@ -43,21 +37,17 @@ static int specifier_prefix_unescaped(char specifier, const void *data, const ch } static int specifier_instance_unescaped(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; - - assert(u); + const Unit *u = ASSERT_PTR(userdata); return unit_name_unescape(strempty(u->instance), ret); } static int specifier_last_component(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; + const Unit *u = ASSERT_PTR(userdata); _cleanup_free_ char *prefix = NULL; char *dash; int r; - assert(u); - r = unit_name_to_prefix(u->id, &prefix); if (r < 0) return r; @@ -82,9 +72,7 @@ static int specifier_last_component_unescaped(char specifier, const void *data, } static int specifier_filename(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; - - assert(u); + const Unit *u = ASSERT_PTR(userdata); if (u->instance) return unit_name_path_unescape(u->instance, ret); @@ -97,11 +85,9 @@ static void bad_specifier(const Unit *u, char specifier) { } static int specifier_cgroup(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; + const Unit *u = ASSERT_PTR(userdata); char *n; - assert(u); - bad_specifier(u, specifier); if (u->cgroup_path) @@ -116,11 +102,9 @@ static int specifier_cgroup(char specifier, const void *data, const char *root, } static int specifier_cgroup_root(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; + const Unit *u = ASSERT_PTR(userdata); char *n; - assert(u); - bad_specifier(u, specifier); n = strdup(u->manager->cgroup_root); @@ -132,11 +116,9 @@ static int specifier_cgroup_root(char specifier, const void *data, const char *r } static int specifier_cgroup_slice(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata, *slice; + const Unit *u = ASSERT_PTR(userdata), *slice; char *n; - assert(u); - bad_specifier(u, specifier); slice = UNIT_GET_SLICE(u); @@ -155,11 +137,9 @@ static int specifier_cgroup_slice(char specifier, const void *data, const char * } static int specifier_special_directory(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const Unit *u = userdata; + const Unit *u = ASSERT_PTR(userdata); char *n = NULL; - assert(u); - n = strdup(u->manager->prefix[PTR_TO_UINT(data)]); if (!n) return -ENOMEM; @@ -169,7 +149,6 @@ static int specifier_special_directory(char specifier, const void *data, const c } int unit_name_printf(const Unit *u, const char* format, char **ret) { - /* * This will use the passed string as format string and replace the following specifiers (which should all be * safe for inclusion in unit names): diff --git a/src/shared/install-printf.c b/src/shared/install-printf.c index 403d6013c1f..14553d8c554 100644 --- a/src/shared/install-printf.c +++ b/src/shared/install-printf.c @@ -14,12 +14,10 @@ #include "user-util.h" static int specifier_prefix_and_instance(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const UnitFileInstallInfo *i = userdata; + const UnitFileInstallInfo *i = ASSERT_PTR(userdata); _cleanup_free_ char *prefix = NULL; int r; - assert(i); - r = unit_name_to_prefix_and_instance(i->name, &prefix); if (r < 0) return r; @@ -38,11 +36,9 @@ static int specifier_prefix_and_instance(char specifier, const void *data, const } static int specifier_name(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const UnitFileInstallInfo *i = userdata; + const UnitFileInstallInfo *i = ASSERT_PTR(userdata); char *ans; - assert(i); - if (unit_name_is_valid(i->name, UNIT_NAME_TEMPLATE) && i->default_instance) return unit_name_replace_instance(i->name, i->default_instance, ret); @@ -54,20 +50,16 @@ static int specifier_name(char specifier, const void *data, const char *root, co } static int specifier_prefix(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const UnitFileInstallInfo *i = userdata; - - assert(i); + const UnitFileInstallInfo *i = ASSERT_PTR(userdata); return unit_name_to_prefix(i->name, ret); } static int specifier_instance(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - const UnitFileInstallInfo *i = userdata; + const UnitFileInstallInfo *i = ASSERT_PTR(userdata); char *instance; int r; - assert(i); - r = unit_name_to_instance(i->name, &instance); if (r < 0) return r; @@ -87,6 +79,8 @@ static int specifier_last_component(char specifier, const void *data, const char char *dash; int r; + assert(ret); + r = specifier_prefix(specifier, data, root, userdata, &prefix); if (r < 0) return r; From 01c69460811f64e416c3e4a545ef84787bb6700b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Jan 2022 15:47:22 +0100 Subject: [PATCH 2/3] shared/specifier: treat NULL the same as "" We would busily allocate an empty string to concatenate all of it's zero characters to the output. Let's make things a bit simpler by letting the specifier functions return NULL to mean "nothing to append". --- src/shared/specifier.c | 56 ++++++++++++++------------------------- src/test/test-specifier.c | 9 ++++--- 2 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/shared/specifier.c b/src/shared/specifier.c index 1fd76b1d15f..f8ab98541fb 100644 --- a/src/shared/specifier.c +++ b/src/shared/specifier.c @@ -35,7 +35,6 @@ int specifier_printf(const char *text, size_t max_length, const Specifier table[], const char *root, const void *userdata, char **ret) { _cleanup_free_ char *result = NULL; bool percent = false; - const char *f; size_t l; char *t; int r; @@ -48,8 +47,10 @@ int specifier_printf(const char *text, size_t max_length, const Specifier table[ return -ENOMEM; t = result; - for (f = text; *f != '\0'; f++, l--) { + for (const char *f = text; *f != '\0'; f++, l--) { if (percent) { + percent = false; + if (*f == '%') *(t++) = '%'; else { @@ -66,6 +67,8 @@ int specifier_printf(const char *text, size_t max_length, const Specifier table[ r = i->lookup(i->specifier, i->data, root, userdata, &w); if (r < 0) return r; + if (isempty(w)) + continue; j = t - result; k = strlen(w); @@ -82,8 +85,6 @@ int specifier_printf(const char *text, size_t max_length, const Specifier table[ *(t++) = *f; } } - - percent = false; } else if (*f == '%') percent = true; else @@ -108,11 +109,13 @@ int specifier_printf(const char *text, size_t max_length, const Specifier table[ /* Generic handler for simple string replacements */ int specifier_string(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - char *n; + char *n = NULL; - n = strdup(strempty(data)); - if (!n) - return -ENOMEM; + if (!isempty(data)) { + n = strdup(data); + if (!n) + return -ENOMEM; + } *ret = n; return 0; @@ -186,10 +189,8 @@ int specifier_short_host_name(char specifier, const void *data, const char *root int specifier_kernel_release(char specifier, const void *data, const char *root, const void *userdata, char **ret) { struct utsname uts; char *n; - int r; - r = uname(&uts); - if (r < 0) + if (uname(&uts) < 0) return -errno; n = strdup(uts.release); @@ -211,47 +212,31 @@ int specifier_architecture(char specifier, const void *data, const char *root, c return 0; } -static int specifier_os_release_common(const char *field, const char *root, char **ret) { - char *t = NULL; - int r; - - r = parse_os_release(root, field, &t); - if (r < 0) - return r; - if (!t) { - /* fields in /etc/os-release might quite possibly be missing, even if everything is entirely - * valid otherwise. Let's hence return "" in that case. */ - t = strdup(""); - if (!t) - return -ENOMEM; - } - - *ret = t; - return 0; -} +/* Note: fields in /etc/os-release might quite possibly be missing, even if everything is entirely valid + * otherwise. We'll return an empty value or NULL in that case from the functions below. */ int specifier_os_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("ID", root, ret); + return parse_os_release(root, "ID", ret); } int specifier_os_version_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("VERSION_ID", root, ret); + return parse_os_release(root, "VERSION_ID", ret); } int specifier_os_build_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("BUILD_ID", root, ret); + return parse_os_release(root, "BUILD_ID", ret); } int specifier_os_variant_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("VARIANT_ID", root, ret); + return parse_os_release(root, "VARIANT_ID", ret); } int specifier_os_image_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("IMAGE_ID", root, ret); + return parse_os_release(root, "IMAGE_ID", ret); } int specifier_os_image_version(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - return specifier_os_release_common("IMAGE_VERSION", root, ret); + return parse_os_release(root, "IMAGE_VERSION", ret); } int specifier_group_name(char specifier, const void *data, const char *root, const void *userdata, char **ret) { @@ -291,7 +276,6 @@ int specifier_user_name(char specifier, const void *data, const char *root, cons } int specifier_user_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - if (asprintf(ret, UID_FMT, getuid()) < 0) return -ENOMEM; diff --git a/src/test/test-specifier.c b/src/test/test-specifier.c index 40957eeb591..dda993ce9d3 100644 --- a/src/test/test-specifier.c +++ b/src/test/test-specifier.c @@ -56,6 +56,7 @@ TEST(specifier_printf) { static const Specifier table[] = { { 'X', specifier_string, (char*) "AAAA" }, { 'Y', specifier_string, (char*) "BBBB" }, + { 'e', specifier_string, NULL }, COMMON_SYSTEM_SPECIFIERS, {} }; @@ -63,21 +64,21 @@ TEST(specifier_printf) { _cleanup_free_ char *w = NULL; int r; - r = specifier_printf("xxx a=%X b=%Y yyy", SIZE_MAX, table, NULL, NULL, &w); + r = specifier_printf("xxx a=%X b=%Y e=%e yyy", SIZE_MAX, table, NULL, NULL, &w); assert_se(r >= 0); assert_se(w); puts(w); - assert_se(streq(w, "xxx a=AAAA b=BBBB yyy")); + assert_se(streq(w, "xxx a=AAAA b=BBBB e= yyy")); free(w); - r = specifier_printf("machine=%m, boot=%b, host=%H, version=%v, arch=%a", SIZE_MAX, table, NULL, NULL, &w); + r = specifier_printf("machine=%m, boot=%b, host=%H, version=%v, arch=%a, empty=%e", SIZE_MAX, table, NULL, NULL, &w); assert_se(r >= 0); assert_se(w); puts(w); w = mfree(w); - specifier_printf("os=%o, os-version=%w, build=%B, variant=%W", SIZE_MAX, table, NULL, NULL, &w); + specifier_printf("os=%o, os-version=%w, build=%B, variant=%W, empty=%e%e%e", SIZE_MAX, table, NULL, NULL, &w); if (w) puts(w); } From 607f032858dd1c123481e37d00391029c5b54001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Jan 2022 16:45:19 +0100 Subject: [PATCH 3/3] core: add %y/%Y specifiers for the fragment path of the unit Fixes #6308: people want to be able to link a unit file via 'systemctl enable' from a git checkout or such and refer to other files in the same repo. The new specifiers make that easy. %y/%Y is used because other more obvious choices like %d/%D or %p/%P are not available because at least on of the two letters is already used. The new specifiers are only available in units. Technically it would be trivial to add then in [Install] too, but I don't see how they could be useful, so I didn't do that. I added both %y and %Y because both were requested in the issue, and because I think both could be useful, depending on the case. %Y to refer to other files in the same repo, and %y in the case where a single repo has multiple unit files, and e.g. each unit has some corresponding asset named after the unit file. --- man/systemd.unit.xml | 10 ++++++++++ src/core/unit-printf.c | 2 ++ src/shared/specifier.c | 22 ++++++++++++++++++++ src/shared/specifier.h | 2 ++ src/test/test-specifier.c | 42 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 2a44b8cfd8e..72a6ba0a7d6 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -2098,6 +2098,16 @@ Note that this setting is not influenced by the Us + + %y + The path to the fragment + This is the path where the main part of the unit file is located. For linked unit files, the real path outside of the unit search directories is used. For units that don't have a fragment file, this specifier will raise an error. + + + %Y + The directory of the fragment + This is the directory part of %y. + diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 774be7ba6f3..4818feef5e0 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -219,6 +219,8 @@ int unit_full_printf_full(const Unit *u, const char *format, size_t max_length, { 'P', specifier_prefix_unescaped, NULL }, { 'f', specifier_filename, NULL }, + { 'y', specifier_real_path, u->fragment_path }, + { 'Y', specifier_real_directory, u->fragment_path }, { 'c', specifier_cgroup, NULL }, { 'r', specifier_cgroup_slice, NULL }, diff --git a/src/shared/specifier.c b/src/shared/specifier.c index f8ab98541fb..aef5b9c94d3 100644 --- a/src/shared/specifier.c +++ b/src/shared/specifier.c @@ -18,6 +18,7 @@ #include "id128-util.h" #include "macro.h" #include "os-util.h" +#include "path-util.h" #include "specifier.h" #include "string-util.h" #include "strv.h" @@ -121,6 +122,27 @@ int specifier_string(char specifier, const void *data, const char *root, const v return 0; } +int specifier_real_path(char specifier, const void *data, const char *root, const void *userdata, char **ret) { + const char *path = data; + + if (!path) + return -ENOENT; + + return chase_symlinks(path, root, 0, ret, NULL); +} + +int specifier_real_directory(char specifier, const void *data, const char *root, const void *userdata, char **ret) { + _cleanup_free_ char *path = NULL; + int r; + + r = specifier_real_path(specifier, data, root, userdata, &path); + if (r < 0) + return r; + + assert(path); + return path_extract_directory(path, ret); +} + int specifier_machine_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { sd_id128_t id; char *n; diff --git a/src/shared/specifier.h b/src/shared/specifier.h index c433ee2d63e..eae5f12ad76 100644 --- a/src/shared/specifier.h +++ b/src/shared/specifier.h @@ -14,6 +14,8 @@ typedef struct Specifier { int specifier_printf(const char *text, size_t max_length, const Specifier table[], const char *root, const void *userdata, char **ret); int specifier_string(char specifier, const void *data, const char *root, const void *userdata, char **ret); +int specifier_real_path(char specifier, const void *data, const char *root, const void *userdata, char **ret); +int specifier_real_directory(char specifier, const void *data, const char *root, const void *userdata, char **ret); int specifier_machine_id(char specifier, const void *data, const char *root, const void *userdata, char **ret); int specifier_boot_id(char specifier, const void *data, const char *root, const void *userdata, char **ret); diff --git a/src/test/test-specifier.c b/src/test/test-specifier.c index dda993ce9d3..ded2dcd55a0 100644 --- a/src/test/test-specifier.c +++ b/src/test/test-specifier.c @@ -3,6 +3,7 @@ #include "alloc-util.h" #include "log.h" #include "specifier.h" +#include "stat-util.h" #include "stdio-util.h" #include "string-util.h" #include "strv.h" @@ -83,6 +84,47 @@ TEST(specifier_printf) { puts(w); } +TEST(specifier_real_path) { + static const Specifier table[] = { + { 'p', specifier_string, "/dev/initctl" }, + { 'y', specifier_real_path, "/dev/initctl" }, + { 'Y', specifier_real_directory, "/dev/initctl" }, + { 'w', specifier_real_path, "/dev/tty" }, + { 'W', specifier_real_directory, "/dev/tty" }, + {} + }; + + _cleanup_free_ char *w = NULL; + int r; + + r = specifier_printf("p=%p y=%y Y=%Y w=%w W=%W", SIZE_MAX, table, NULL, NULL, &w); + assert_se(r >= 0 || r == -ENOENT); + assert_se(w || r == -ENOENT); + puts(strnull(w)); + + /* /dev/initctl should normally be a symlink to /run/initctl */ + if (files_same("/dev/initctl", "/run/initctl", 0) > 0) + assert_se(streq(w, "p=/dev/initctl y=/run/initctl Y=/run w=/dev/tty W=/dev")); +} + +TEST(specifier_real_path_missing_file) { + static const Specifier table[] = { + { 'p', specifier_string, "/dev/-no-such-file--" }, + { 'y', specifier_real_path, "/dev/-no-such-file--" }, + { 'Y', specifier_real_directory, "/dev/-no-such-file--" }, + {} + }; + + _cleanup_free_ char *w = NULL; + int r; + + r = specifier_printf("p=%p y=%y", SIZE_MAX, table, NULL, NULL, &w); + assert_se(r == -ENOENT); + + r = specifier_printf("p=%p Y=%Y", SIZE_MAX, table, NULL, NULL, &w); + assert_se(r == -ENOENT); +} + TEST(specifiers) { for (const Specifier *s = specifier_table; s->specifier; s++) { char spec[3];