From 6255bbe2624caa1e656c4f8a1b88721b62c2e16e Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Thu, 23 Mar 2023 10:47:06 +0000 Subject: [PATCH 1/7] env: add load_env_file_pairs_fd() --- src/basic/env-file.c | 14 ++++++++++++++ src/basic/env-file.h | 1 + src/portable/portable.c | 8 +------- src/test/test-fileio.c | 36 +++++++++++++++++++++++------------- 4 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/basic/env-file.c b/src/basic/env-file.c index 34027ca2e5..01ed443d5f 100644 --- a/src/basic/env-file.c +++ b/src/basic/env-file.c @@ -478,6 +478,7 @@ int load_env_file_pairs(FILE *f, const char *fname, char ***ret) { int r; assert(f || fname); + assert(ret); r = parse_env_file_internal(f, fname, load_env_file_push_pairs, &m); if (r < 0) @@ -487,6 +488,19 @@ int load_env_file_pairs(FILE *f, const char *fname, char ***ret) { return 0; } +int load_env_file_pairs_fd(int fd, const char *fname, char ***ret) { + _cleanup_fclose_ FILE *f = NULL; + int r; + + assert(fd >= 0); + + r = fdopen_independent(fd, "re", &f); + if (r < 0) + return r; + + return load_env_file_pairs(f, fname, ret); +} + static int merge_env_file_push( const char *filename, unsigned line, const char *key, char *value, diff --git a/src/basic/env-file.h b/src/basic/env-file.h index dc38b7a5c9..fa22d2209c 100644 --- a/src/basic/env-file.h +++ b/src/basic/env-file.h @@ -14,6 +14,7 @@ int parse_env_file_fd_sentinel(int fd, const char *fname, ...) _sentinel_; #define parse_env_file_fd(fd, fname, ...) parse_env_file_fd_sentinel(fd, fname, __VA_ARGS__, NULL) int load_env_file(FILE *f, const char *fname, char ***ret); int load_env_file_pairs(FILE *f, const char *fname, char ***ret); +int load_env_file_pairs_fd(int fd, const char *fname, char ***ret); int merge_env_file(char ***env, FILE *f, const char *fname); diff --git a/src/portable/portable.c b/src/portable/portable.c index 8a3211166e..770cac9e1f 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -594,7 +594,6 @@ static int extract_image_and_extensions( _cleanup_(portable_metadata_unrefp) PortableMetadata *extension_release_meta = NULL; _cleanup_hashmap_free_ Hashmap *extra_unit_files = NULL; _cleanup_strv_free_ char **extension_release = NULL; - _cleanup_fclose_ FILE *f = NULL; const char *e; r = portable_extract_by_path(ext->path, /* path_is_extension= */ true, relax_extension_release_check, matches, &extension_release_meta, &extra_unit_files, error); @@ -608,12 +607,7 @@ static int extract_image_and_extensions( if (!validate_sysext && !ret_valid_prefixes && !ret_extension_releases) continue; - /* We need to keep the fd valid, to return the PortableMetadata to the caller. */ - r = fdopen_independent(extension_release_meta->fd, "re", &f); - if (r < 0) - return r; - - r = load_env_file_pairs(f, extension_release_meta->name, &extension_release); + r = load_env_file_pairs_fd(extension_release_meta->fd, extension_release_meta->name, &extension_release); if (r < 0) return r; diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 1a9a8a5ddc..d8d622e44d 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -509,6 +509,23 @@ TEST(write_string_file_verify) { assert_se(write_string_file("/proc/version", buf2, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_AVOID_NEWLINE) == 0); } +static void check_file_pairs_one(char **l) { + assert_se(l); + assert_se(strv_length(l) == 14); + + STRV_FOREACH_PAIR(k, v, l) { + assert_se(STR_IN_SET(*k, "NAME", "ID", "PRETTY_NAME", "ANSI_COLOR", "HOME_URL", "SUPPORT_URL", "BUG_REPORT_URL")); + printf("%s=%s\n", *k, *v); + assert_se(!streq(*k, "NAME") || streq(*v, "Arch Linux")); + assert_se(!streq(*k, "ID") || streq(*v, "arch")); + assert_se(!streq(*k, "PRETTY_NAME") || streq(*v, "Arch Linux")); + assert_se(!streq(*k, "ANSI_COLOR") || streq(*v, "0;36")); + assert_se(!streq(*k, "HOME_URL") || streq(*v, "https://www.archlinux.org/")); + assert_se(!streq(*k, "SUPPORT_URL") || streq(*v, "https://bbs.archlinux.org/")); + assert_se(!streq(*k, "BUG_REPORT_URL") || streq(*v, "https://bugs.archlinux.org/")); + } +} + TEST(load_env_file_pairs) { _cleanup_(unlink_tempfilep) char fn[] = "/tmp/test-load_env_file_pairs-XXXXXX"; int fd, r; @@ -529,24 +546,17 @@ TEST(load_env_file_pairs) { WRITE_STRING_FILE_CREATE); assert_se(r == 0); + r = load_env_file_pairs_fd(fd, fn, &l); + assert_se(r >= 0); + check_file_pairs_one(l); + l = strv_free(l); + f = fdopen(fd, "r"); assert_se(f); r = load_env_file_pairs(f, fn, &l); assert_se(r >= 0); - - assert_se(strv_length(l) == 14); - STRV_FOREACH_PAIR(k, v, l) { - assert_se(STR_IN_SET(*k, "NAME", "ID", "PRETTY_NAME", "ANSI_COLOR", "HOME_URL", "SUPPORT_URL", "BUG_REPORT_URL")); - printf("%s=%s\n", *k, *v); - if (streq(*k, "NAME")) assert_se(streq(*v, "Arch Linux")); - if (streq(*k, "ID")) assert_se(streq(*v, "arch")); - if (streq(*k, "PRETTY_NAME")) assert_se(streq(*v, "Arch Linux")); - if (streq(*k, "ANSI_COLOR")) assert_se(streq(*v, "0;36")); - if (streq(*k, "HOME_URL")) assert_se(streq(*v, "https://www.archlinux.org/")); - if (streq(*k, "SUPPORT_URL")) assert_se(streq(*v, "https://bbs.archlinux.org/")); - if (streq(*k, "BUG_REPORT_URL")) assert_se(streq(*v, "https://bugs.archlinux.org/")); - } + check_file_pairs_one(l); } TEST(search_and_fopen) { From 2ed74695b310207deca33fcc95a890b0fdb9e622 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Mon, 27 Mar 2023 12:05:28 +0100 Subject: [PATCH 2/7] strv: add helper to find value in key/value pairs from list of keys --- src/basic/strv.c | 11 +++++++++++ src/basic/strv.h | 3 +++ src/test/test-strv.c | 12 ++++++++++++ 3 files changed, 26 insertions(+) diff --git a/src/basic/strv.c b/src/basic/strv.c index 5fcf3620a6..822dadeeb4 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -7,6 +7,7 @@ #include #include "alloc-util.h" +#include "env-util.h" #include "escape.h" #include "extract-word.h" #include "fileio.h" @@ -63,6 +64,16 @@ char* strv_find_startswith(char * const *l, const char *name) { return NULL; } +char* strv_find_first_field(char * const *needles, char * const *haystack) { + STRV_FOREACH(k, needles) { + char *value = strv_env_pairs_get((char **)haystack, *k); + if (value) + return value; + } + + return NULL; +} + char** strv_free(char **l) { STRV_FOREACH(k, l) free(*k); diff --git a/src/basic/strv.h b/src/basic/strv.h index 419cda1ee3..b4d3f121f9 100644 --- a/src/basic/strv.h +++ b/src/basic/strv.h @@ -17,6 +17,9 @@ char* strv_find(char * const *l, const char *name) _pure_; char* strv_find_case(char * const *l, const char *name) _pure_; char* strv_find_prefix(char * const *l, const char *name) _pure_; char* strv_find_startswith(char * const *l, const char *name) _pure_; +/* Given two vectors, the first a list of keys and the second a list of key-value pairs, returns the value + * of the first key from the first vector that is found in the second vector. */ +char* strv_find_first_field(char * const *needles, char * const *haystack) _pure_; #define strv_contains(l, s) (!!strv_find((l), (s))) #define strv_contains_case(l, s) (!!strv_find_case((l), (s))) diff --git a/src/test/test-strv.c b/src/test/test-strv.c index 0f08dd4615..a39a2d8122 100644 --- a/src/test/test-strv.c +++ b/src/test/test-strv.c @@ -994,4 +994,16 @@ TEST(strv_copy_n) { assert_se(strv_equal(l, STRV_MAKE("a", "b", "c", "d", "e"))); } +TEST(strv_find_first_field) { + char **haystack = STRV_MAKE("a", "b", "c", "d", "e", "f", "g", "h", "i", "j"); + + assert_se(strv_find_first_field(NULL, NULL) == NULL); + assert_se(strv_find_first_field(NULL, haystack) == NULL); + assert_se(strv_find_first_field(STRV_MAKE("k", "l", "m", "d", "b"), NULL) == NULL); + assert_se(strv_find_first_field(STRV_MAKE("k", "l", "m", "d", "b"), haystack) == NULL); + assert_se(streq_ptr(strv_find_first_field(STRV_MAKE("k", "l", "m", "d", "a", "c"), haystack), "b")); + assert_se(streq_ptr(strv_find_first_field(STRV_MAKE("k", "l", "m", "d", "c", "a"), haystack), "d")); + assert_se(streq_ptr(strv_find_first_field(STRV_MAKE("i", "k", "l", "m", "d", "c", "a", "b"), haystack), "j")); +} + DEFINE_TEST_MAIN(LOG_INFO); From 62b7c23f79bcbcbc141c90c3a0931fa46a1652a2 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Thu, 23 Mar 2023 01:02:47 +0000 Subject: [PATCH 3/7] portable: use parse_env_file_fd to keep FD valid take_fdopen_unlocked invalidates the FD in the PortableMetadata object, so it cannot be used later. Use parse_env_file_fd instead which is non destructive. --- src/portable/portable.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index 770cac9e1f..2b06e61f75 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -566,18 +566,13 @@ static int extract_image_and_extensions( * extension-release metadata match, otherwise reject it immediately as invalid, or it will fail when * the units are started. Also, collect valid portable prefixes if caller requested that. */ if (validate_sysext || ret_valid_prefixes) { - _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *prefixes = NULL; - r = take_fdopen_unlocked(&os_release->fd, "r", &f); - if (r < 0) - return r; - - r = parse_env_file(f, os_release->name, - "ID", &id, - "VERSION_ID", &version_id, - "SYSEXT_LEVEL", &sysext_level, - "PORTABLE_PREFIXES", &prefixes); + r = parse_env_file_fd(os_release->fd, os_release->name, + "ID", &id, + "VERSION_ID", &version_id, + "SYSEXT_LEVEL", &sysext_level, + "PORTABLE_PREFIXES", &prefixes); if (r < 0) return r; if (isempty(id)) From 8c8331fc5075c2d82b13fb121b2571a12d1c54a0 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 22 Mar 2023 22:44:31 +0000 Subject: [PATCH 4/7] portable: include base and extension images in log fields When a portable service uses extensions, we use the 'main' image name (the one where the unit was found in) as PORTABLE=. It is useful to also list all the images actually used at runtime, as they might contain libraries and so on. Use PORTABLE_ROOT= for the image/directory that is used as RootImage= or RootDirectory=, and PORTABLE_EXTENSION= for the image/directory that is used as ExtensionImages= or ExtensionDirectories=. Note that these new fields are only added if extensions are used, there's no change for single-DDI portables. Example with a base and two extensions, with the unit coming from the first extension: [Service] RootImage=/home/bluca/git/systemd/base.raw Environment=PORTABLE=app0.raw BindReadOnlyPaths=/etc/os-release:/run/host/os-release LogExtraFields=PORTABLE=app0.raw LogExtraFields=PORTABLE_ROOT=base.raw ExtensionImages=/home/bluca/git/systemd/app0.raw LogExtraFields=PORTABLE_EXTENSION=app0.raw ExtensionImages=/home/bluca/git/systemd/app1.raw LogExtraFields=PORTABLE_EXTENSION=app1.raw --- docs/PORTABLE_SERVICES.md | 21 +++++++++++++++++++++ src/portable/portable.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/docs/PORTABLE_SERVICES.md b/docs/PORTABLE_SERVICES.md index b12e7754e7..7936eebccb 100644 --- a/docs/PORTABLE_SERVICES.md +++ b/docs/PORTABLE_SERVICES.md @@ -335,6 +335,27 @@ service data may be placed on the host file system. Use `StateDirectory=` in the unit files to enable such behaviour and add a local data directory to the services copied onto the host. +## Logging + +Several fields are autotmatically added to log messages generated by a portable +service (or about a portable service, e.g.: start/stop logs from systemd). +The `PORTABLE=` field will refer to the name of the portable image where the unit +was loaded from. In case extensions are used, additionally there will be a +`PORTABLE_ROOT=` field, referring to the name of image used as the base layer +(i.e.: `RootImage=` or `RootDirectory=`), and one `PORTABLE_EXTENSION=` field per +each extension image used. + +For example, a portable service `app0` using two extensions `app0.raw` and +`app1.raw`, and a base layer `base.raw`, will create log entries with the +following fields: + +``` +PORTABLE=app0.raw +PORTABLE_ROOT=base.raw +PORTABLE_EXTENSION=app0.raw +PORTABLE_EXTENSION=app1.raw +``` + ## Links [`portablectl(1)`](https://www.freedesktop.org/software/systemd/man/portablectl.html)
diff --git a/src/portable/portable.c b/src/portable/portable.c index 2b06e61f75..d19253debb 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -994,19 +994,41 @@ static int install_chroot_dropin( "LogExtraFields=PORTABLE=", base_name, "\n")) return -ENOMEM; + if (!ordered_hashmap_isempty(extension_images)) { + _cleanup_free_ char *root_base_name = NULL; + + r = path_extract_filename(image_path, &root_base_name); + if (r < 0) + return log_debug_errno(r, "Failed to extract basename from '%s': %m", image_path); + + if (!strextend(&text, "LogExtraFields=PORTABLE_ROOT=", root_base_name, "\n")) + return -ENOMEM; + } + if (m->image_path && !path_equal(m->image_path, image_path)) - ORDERED_HASHMAP_FOREACH(ext, extension_images) + ORDERED_HASHMAP_FOREACH(ext, extension_images) { + _cleanup_free_ char *extension_base_name = NULL; + + r = path_extract_filename(ext->path, &extension_base_name); + if (r < 0) + return log_debug_errno(r, "Failed to extract basename from '%s': %m", ext->path); + if (!strextend(&text, + "\n", extension_setting_from_image(ext->type), ext->path, /* With --force tell PID1 to avoid enforcing that the image and * extension-release. have to match. */ !IN_SET(type, IMAGE_DIRECTORY, IMAGE_SUBVOLUME) && FLAGS_SET(flags, PORTABLE_FORCE_SYSEXT) ? - ":x-systemd.relax-extension-release-check" : - "", - "\n")) + ":x-systemd.relax-extension-release-check\n" : + "\n", + /* In PORTABLE= we list the 'main' image name for this unit + * (the image where the unit was extracted from), but we are + * stacking multiple images, so list those too. */ + "LogExtraFields=PORTABLE_EXTENSION=", extension_base_name, "\n")) return -ENOMEM; + } } r = write_string_file(dropin, text, WRITE_STRING_FILE_CREATE|WRITE_STRING_FILE_ATOMIC); From e8114a4f86efa9a176962bbebbba4cb8b5a1c322 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Thu, 23 Mar 2023 01:23:04 +0000 Subject: [PATCH 5/7] portable: add PORTABLE_NAME_AND_VERSION= and other metadata to LogsExtraFields= This is useful to identify log messages with metadata from the images they run on. Look for ID/VERSION_ID/IMAGE_ID/IMAGE_VERSION/BUILD_ID, with a SYSEXT_ prefix if we are looking at an extension, and append via LogExtraFields= as respectively PORTABLE_NAME_AND_VERSION= in case of a single image. In case of extensions, append as PORTABLE_ROOT_NAME_AND_VERSION= for the base and one PORTABLE_EXTENSION_AND_VERSION= for each extension. Example with a base and two extensions, with the unit coming from the first extension: [Service] RootImage=/home/bluca/git/systemd/base.raw Environment=PORTABLE=app0.raw BindReadOnlyPaths=/etc/os-release:/run/host/os-release LogExtraFields=PORTABLE=app0.raw Environment=PORTABLE_ROOT=base.raw LogExtraFields=PORTABLE_ROOT=base.raw LogExtraFields=PORTABLE_ROOT_NAME_AND_VERSION=debian_10 ExtensionImages=/home/bluca/git/systemd/app0.raw LogExtraFields=PORTABLE_EXTENSION=app0.raw LogExtraFields=PORTABLE_EXTENSION_NAME_AND_VERSION=app_0 ExtensionImages=/home/bluca/git/systemd/app1.raw LogExtraFields=PORTABLE_EXTENSION=app1.raw LogExtraFields=PORTABLE_EXTENSION_NAME_AND_VERSION=app_1 --- docs/PORTABLE_SERVICES.md | 22 +++++++- src/portable/portable.c | 101 ++++++++++++++++++++++++++++++++++--- test/test-functions | 4 ++ test/units/testsuite-29.sh | 22 ++++++++ 4 files changed, 139 insertions(+), 10 deletions(-) diff --git a/docs/PORTABLE_SERVICES.md b/docs/PORTABLE_SERVICES.md index 7936eebccb..8d65c9002d 100644 --- a/docs/PORTABLE_SERVICES.md +++ b/docs/PORTABLE_SERVICES.md @@ -345,15 +345,33 @@ was loaded from. In case extensions are used, additionally there will be a (i.e.: `RootImage=` or `RootDirectory=`), and one `PORTABLE_EXTENSION=` field per each extension image used. +The `os-release` file from the portable image will be parsed and added as structured +metadata to the journal log entries. The parsed fields will be the first ID field which +is set from the set of `IMAGE_ID` and `ID` in this order of preference, and the first +version field which is set from a set of `IMAGE_VERSION`, `VERSION_ID`, and `BUILD_ID` +in this order of preference. The ID and version, if any, are concatenated with an +underscore (`_`) as separator. If only either one is found, it will be used by itself. +The field will be named `PORTABLE_NAME_AND_VERSION=`. + +In case extensions are used, the same fields in the same order are, but prefixed by +`SYSEXT_`, are parsed from each `extension-release` file, and are appended to the +journal as log entries, using `PORTABLE_EXTENSION_NAME_AND_VERSION=` as the field +name. The base layer's field will be named `PORTABLE_ROOT_NAME_AND_VERSION=` instead +of `PORTABLE_NAME_AND_VERSION=` in this case. + For example, a portable service `app0` using two extensions `app0.raw` and -`app1.raw`, and a base layer `base.raw`, will create log entries with the -following fields: +`app1.raw` (with `SYSEXT_ID=app`, and `SYSEXT_VERSION_ID=` `0` and `1` in their +respective extension-releases), and a base layer `base.raw` (with `VERSION_ID=10` and +`ID=debian` in `os-release`), will create log entries with the following fields: ``` PORTABLE=app0.raw PORTABLE_ROOT=base.raw +PORTABLE_ROOT_NAME_AND_VERSION=debian_10 PORTABLE_EXTENSION=app0.raw +PORTABLE_EXTENSION_NAME_AND_VERSION=app_0 PORTABLE_EXTENSION=app1.raw +PORTABLE_EXTENSION_NAME_AND_VERSION=app_1 ``` ## Links diff --git a/src/portable/portable.c b/src/portable/portable.c index d19253debb..dbebebf4ce 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -940,11 +940,67 @@ static int make_marker_text(const char *image_path, OrderedHashmap *extension_im return 0; } +static int append_release_log_fields( + char **text, + const PortableMetadata *release, + ImageClass type, + const char *field_name) { + + static const char *const field_versions[_IMAGE_CLASS_MAX][4]= { + [IMAGE_PORTABLE] = { "IMAGE_VERSION", "VERSION_ID", "BUILD_ID", NULL }, + [IMAGE_EXTENSION] = { "SYSEXT_IMAGE_VERSION", "SYSEXT_VERSION_ID", "SYSEXT_BUILD_ID", NULL }, + }; + static const char *const field_ids[_IMAGE_CLASS_MAX][3]= { + [IMAGE_PORTABLE] = { "IMAGE_ID", "ID", NULL }, + [IMAGE_EXTENSION] = { "SYSEXT_IMAGE_ID", "SYSEXT_ID", NULL }, + }; + _cleanup_strv_free_ char **fields = NULL; + const char *id = NULL, *version = NULL; + int r; + + assert(IN_SET(type, IMAGE_PORTABLE, IMAGE_EXTENSION)); + assert(!strv_isempty((char *const *)field_ids[type])); + assert(!strv_isempty((char *const *)field_versions[type])); + assert(field_name); + assert(text); + + if (!release) + return 0; /* Nothing to do. */ + + r = load_env_file_pairs_fd(release->fd, release->name, &fields); + if (r < 0) + return log_debug_errno(r, "Failed to parse '%s': %m", release->name); + + /* Find an ID first, in order of preference from more specific to less specific: IMAGE_ID -> ID */ + id = strv_find_first_field((char *const *)field_ids[type], fields); + + /* Then the version, same logic, prefer the more specific one */ + version = strv_find_first_field((char *const *)field_versions[type], fields); + + /* If there's no valid version to be found, simply omit it. */ + if (!id && !version) + return 0; + + if (!strextend(text, + "LogExtraFields=", + field_name, + "=", + strempty(id), + id && version ? "_" : "", + strempty(version), + "\n")) + return -ENOMEM; + + return 0; +} + static int install_chroot_dropin( const char *image_path, ImageType type, OrderedHashmap *extension_images, + OrderedHashmap *extension_releases, const PortableMetadata *m, + const PortableMetadata *os_release, const char *dropin_dir, PortableFlags flags, char **ret_dropin, @@ -994,16 +1050,30 @@ static int install_chroot_dropin( "LogExtraFields=PORTABLE=", base_name, "\n")) return -ENOMEM; - if (!ordered_hashmap_isempty(extension_images)) { + /* If we have a single image then PORTABLE= will point to it, so we add + * PORTABLE_NAME_AND_VERSION= with the os-release fields and we are done. But if we have + * extensions, PORTABLE= will point to the image where the current unit was found in. So we + * also list PORTABLE_ROOT= and PORTABLE_ROOT_NAME_AND_VERSION= for the base image, and + * PORTABLE_EXTENSION= and PORTABLE_EXTENSION_NAME_AND_VERSION= for each extension, so that + * all needed metadata is available. */ + if (ordered_hashmap_isempty(extension_images)) + r = append_release_log_fields(&text, os_release, IMAGE_PORTABLE, "PORTABLE_NAME_AND_VERSION"); + else { _cleanup_free_ char *root_base_name = NULL; r = path_extract_filename(image_path, &root_base_name); if (r < 0) return log_debug_errno(r, "Failed to extract basename from '%s': %m", image_path); - if (!strextend(&text, "LogExtraFields=PORTABLE_ROOT=", root_base_name, "\n")) + if (!strextend(&text, + "Environment=PORTABLE_ROOT=", root_base_name, "\n", + "LogExtraFields=PORTABLE_ROOT=", root_base_name, "\n")) return -ENOMEM; + + r = append_release_log_fields(&text, os_release, IMAGE_PORTABLE, "PORTABLE_ROOT_NAME_AND_VERSION"); } + if (r < 0) + return r; if (m->image_path && !path_equal(m->image_path, image_path)) ORDERED_HASHMAP_FOREACH(ext, extension_images) { @@ -1028,6 +1098,18 @@ static int install_chroot_dropin( * stacking multiple images, so list those too. */ "LogExtraFields=PORTABLE_EXTENSION=", extension_base_name, "\n")) return -ENOMEM; + + /* Look for image/version identifiers in the extension release files. We + * look for all possible IDs, but typically only 1 or 2 will be set, so + * the number of fields added shouldn't be too large. We prefix the DDI + * name to the value, so that we can add the same field multiple times and + * still be able to identify what applies to what. */ + r = append_release_log_fields(&text, + ordered_hashmap_get(extension_releases, ext->name), + IMAGE_EXTENSION, + "PORTABLE_EXTENSION_NAME_AND_VERSION"); + if (r < 0) + return r; } } @@ -1117,7 +1199,9 @@ static int attach_unit_file( const char *image_path, ImageType type, OrderedHashmap *extension_images, + OrderedHashmap *extension_releases, const PortableMetadata *m, + const PortableMetadata *os_release, const char *profile, PortableFlags flags, PortableChange **changes, @@ -1161,7 +1245,7 @@ static int attach_unit_file( * is reloaded while we are creating things here: as long as only the drop-ins exist the unit doesn't exist at * all for PID 1. */ - r = install_chroot_dropin(image_path, type, extension_images, m, dropin_dir, flags, &chroot_dropin, changes, n_changes); + r = install_chroot_dropin(image_path, type, extension_images, extension_releases, m, os_release, dropin_dir, flags, &chroot_dropin, changes, n_changes); if (r < 0) return r; @@ -1313,7 +1397,8 @@ int portable_attach( size_t *n_changes, sd_bus_error *error) { - _cleanup_ordered_hashmap_free_ OrderedHashmap *extension_images = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *extension_images = NULL, *extension_releases = NULL; + _cleanup_(portable_metadata_unrefp) PortableMetadata *os_release = NULL; _cleanup_hashmap_free_ Hashmap *unit_files = NULL; _cleanup_(lookup_paths_free) LookupPaths paths = {}; _cleanup_strv_free_ char **valid_prefixes = NULL; @@ -1329,8 +1414,8 @@ int portable_attach( /* relax_extension_release_check= */ FLAGS_SET(flags, PORTABLE_FORCE_SYSEXT), &image, &extension_images, - /* extension_releases= */ NULL, - /* os_release= */ NULL, + &extension_releases, + &os_release, &unit_files, &valid_prefixes, error); @@ -1397,8 +1482,8 @@ int portable_attach( } HASHMAP_FOREACH(item, unit_files) { - r = attach_unit_file(&paths, image->path, image->type, extension_images, - item, profile, flags, changes, n_changes); + r = attach_unit_file(&paths, image->path, image->type, extension_images, extension_releases, + item, os_release, profile, flags, changes, n_changes); if (r < 0) return sd_bus_error_set_errnof(error, r, "Failed to attach unit '%s': %m", item->name); } diff --git a/test/test-functions b/test/test-functions index e0cc6a85a5..5a16459aeb 100644 --- a/test/test-functions +++ b/test/test-functions @@ -685,6 +685,8 @@ EOF mkdir -p "$initdir/usr/lib/extension-release.d" "$initdir/usr/lib/systemd/system" "$initdir/opt" grep "^ID=" "$os_release" >"$initdir/usr/lib/extension-release.d/extension-release.app0" echo "${version_id}" >>"$initdir/usr/lib/extension-release.d/extension-release.app0" + ( echo "${version_id}" + echo "SYSEXT_IMAGE_ID=app" ) >>"$initdir/usr/lib/extension-release.d/extension-release.app0" cat >"$initdir/usr/lib/systemd/system/app0.service" <"$initdir/usr/lib/extension-release.d/extension-release.app2" ( echo "${version_id}" echo "SYSEXT_SCOPE=portable" + echo "SYSEXT_IMAGE_ID=app" + echo "SYSEXT_IMAGE_VERSION=1" echo "PORTABLE_PREFIXES=app1" ) >>"$initdir/usr/lib/extension-release.d/extension-release.app2" setfattr -n user.extension-release.strict -v false "$initdir/usr/lib/extension-release.d/extension-release.app2" cat >"$initdir/usr/lib/systemd/system/app1.service" < Date: Tue, 28 Mar 2023 10:36:49 +0100 Subject: [PATCH 6/7] portablectl: display sysext-specific fields The wrong fields were being displayed, if at all. ID and VERSION_ID in sysexts are used for matching, they don't identify the sysext itself. Parse the newly defined fields and display them separately from the compatibility fields. Before: Image: /home/bluca/git/systemd/base.raw Portable Service: n/a Operating System: Debian GNU/Linux 10 (buster) Extension: /home/bluca/git/systemd/app0.raw Extension Scope: n/a Extension Compatibility Level: n/a Portable Service: n/a Portable Prefixes: n/a Operating System: n/a (debian 10) Extension: /home/bluca/git/systemd/app1.raw Extension Scope: n/a Extension Compatibility Level: n/a Portable Service: n/a Portable Prefixes: n/a Operating System: n/a (debian 10) Unit files: app0.service app1.service After: Image: /home/bluca/git/systemd/base.raw Portable Service: n/a Operating System: Debian GNU/Linux 10 (buster) Extension: /home/bluca/git/systemd/app0.raw Extension Scope: n/a Extension Compatibility Level: n/a Extension Compatibility OS: debian Extension Compatibility OS Version: 10 Portable Service: n/a Portable Prefixes: n/a Extension Image: ID: app Version: 0 Extension: /home/bluca/git/systemd/app1.raw Extension Scope: n/a Extension Compatibility Level: n/a Extension Compatibility OS: debian Extension Compatibility OS Version: 10 Portable Service: n/a Portable Prefixes: n/a Extension Image: ID: app Version: 1 Unit files: app0.service app1.service --- src/portable/portablectl.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/portable/portablectl.c b/src/portable/portablectl.c index d390a9295d..d463eb4120 100644 --- a/src/portable/portablectl.c +++ b/src/portable/portablectl.c @@ -400,7 +400,8 @@ static int inspect_image(int argc, char *argv[], void *userdata) { nl = true; } else { _cleanup_free_ char *pretty_portable = NULL, *pretty_os = NULL, *sysext_level = NULL, - *id = NULL, *version_id = NULL, *sysext_scope = NULL, *portable_prefixes = NULL; + *sysext_id = NULL, *sysext_version_id = NULL, *sysext_scope = NULL, *portable_prefixes = NULL, + *id = NULL, *version_id = NULL, *image_id = NULL, *image_version = NULL, *build_id = NULL; _cleanup_fclose_ FILE *f = NULL; f = fmemopen_unlocked((void*) data, sz, "r"); @@ -408,30 +409,42 @@ static int inspect_image(int argc, char *argv[], void *userdata) { return log_error_errno(errno, "Failed to open extension-release buffer: %m"); r = parse_env_file(f, name, - "ID", &id, - "VERSION_ID", &version_id, + "SYSEXT_ID", &sysext_id, + "SYSEXT_VERSION_ID", &sysext_version_id, + "SYSEXT_BUILD_ID", &build_id, + "SYSEXT_IMAGE_ID", &image_id, + "SYSEXT_IMAGE_VERSION", &image_version, + "SYSEXT_PRETTY_NAME", &pretty_os, "SYSEXT_SCOPE", &sysext_scope, "SYSEXT_LEVEL", &sysext_level, + "ID", &id, + "VERSION_ID", &version_id, "PORTABLE_PRETTY_NAME", &pretty_portable, - "PORTABLE_PREFIXES", &portable_prefixes, - "PRETTY_NAME", &pretty_os); + "PORTABLE_PREFIXES", &portable_prefixes); if (r < 0) return log_error_errno(r, "Failed to parse extension release from '%s': %m", name); printf("Extension:\n\t%s\n" "\tExtension Scope:\n\t\t%s\n" "\tExtension Compatibility Level:\n\t\t%s\n" + "\tExtension Compatibility OS:\n\t\t%s\n" + "\tExtension Compatibility OS Version:\n\t\t%s\n" "\tPortable Service:\n\t\t%s\n" "\tPortable Prefixes:\n\t\t%s\n" - "\tOperating System:\n\t\t%s (%s %s)\n", + "\tExtension Image:\n\t\t%s%s%s %s%s%s\n", name, strna(sysext_scope), strna(sysext_level), + strna(id), + strna(version_id), strna(pretty_portable), strna(portable_prefixes), - strna(pretty_os), - strna(id), - strna(version_id)); + strempty(pretty_os), + pretty_os ? " (" : "ID: ", + strna(sysext_id ?: image_id), + pretty_os ? "" : "Version: ", + strna(sysext_version_id ?: image_version ?: build_id), + pretty_os ? ")" : ""); } r = sd_bus_message_exit_container(reply); From 38d1d1046515a0ef8d10ef732bbafffc1ad54c13 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 24 Mar 2023 15:10:11 +0000 Subject: [PATCH 7/7] os-release: add 'SYSEXT_' fields for version/id sysext DDI cannot carry an os-release file, but have to carry an extension-release file. But so far, this was only used to match the sysext DDI with the base DDI/rootdir. It is also useful to describe the sysext DDI itself, just like we do in os-release. So document that the same fields used in os-release can also be added to an extension-release, with the 'SYSEXT_' prefix, and in that case they are understood to define the sysext DDI itself, rather than for matching purposes. --- man/os-release.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/man/os-release.xml b/man/os-release.xml index 113ef9fc18..e74f27b990 100644 --- a/man/os-release.xml +++ b/man/os-release.xml @@ -111,6 +111,11 @@ VERSION_ID= exists and matches. This ensures ABI/API compatibility between the layers and prevents merging of an incompatible image in an overlay. + In order to identify the extension image itself, the same fields defined below can be added to the + extension-release file with a SYSEXT_ prefix (to disambiguate + from fields used to match on the base image). E.g.: SYSEXT_ID=myext, + SYSEXT_VERSION_ID=1.2.3. + In the extension-release.IMAGE filename, the IMAGE part must exactly match the file name of the containing image with the suffix removed. In case it is not possible to guarantee that an image file name is stable and doesn't