Merge pull request #21908 from yonran/environmentfile-docs

man: clarify Environmentfile format

docs/PORTABLE_SERVICES.md

@@ -284,9 +284,12 @@ following must be also be observed:
 4. The upper extension(s) image(s) must at least contain one matching unit file each,
    with the right name prefix and suffix (see above).
 
+5. As with the base/OS image, the upper extension(s) image(s) must be a plain
+   sub-directory, a btrfs subvolume or a raw disk image.
+
 ```
 # portablectl attach --extension foobar_0.7.23.raw debian-runtime_11.1.raw foobar
-# portablectl attach --extension barbaz_7.0.23.raw debian-runtime_11.1.raw barbaz
+# portablectl attach --extension barbaz_7.0.23/ debian-runtime_11.1.raw barbaz
 ```
 
 ## Execution Environment

man/portablectl.xml

@@ -365,8 +365,9 @@
         The image(s) must contain an <filename>extension-release</filename> file with metadata that matches
         what is defined in the <filename>os-release</filename> of <replaceable>IMAGE</replaceable>. See:
         <citerefentry><refentrytitle>os-release</refentrytitle><manvolnum>5</manvolnum></citerefentry>.
-        For more information on portable services with extensions, see the <literal>Extension Images</literal>
+        Images can be block images, btrfs subvolumes or directories. For more information on portable
-        paragraph on <ulink url="https://systemd.io/PORTABLE_SERVICES">Portable Services</ulink>.
+        services with extensions, see the <literal>Extension Images</literal> paragraph on
+        <ulink url="https://systemd.io/PORTABLE_SERVICES">Portable Services</ulink>.
         </para>
 
         <para>Note that the same extensions have to be specified, in the same order, when attaching

man/systemd.exec.xml

@@ -2513,18 +2513,39 @@ SystemCallErrorNumber=EPERM</programlisting>
       <varlistentry>
         <term><varname>EnvironmentFile=</varname></term>
 
-        <listitem><para>Similar to <varname>Environment=</varname> but reads the environment variables from a text
+        <listitem><para>Similar to <varname>Environment=</varname> but reads the environment variables from a text file.
-        file. The text file should contain new-line-separated variable assignments.  Empty lines, lines without an
+        The text file should contain newline-separated variable assignments.  Empty lines, lines without an
-        <literal>=</literal> separator, or lines starting with ; or # will be ignored, which may be used for
+        <literal>=</literal> separator, or lines starting with <literal>;</literal> or <literal>#</literal> will be
-        commenting. A line ending with a backslash will be concatenated with the following one, allowing multiline
+        ignored, which may be used for commenting. The file must be UTF-8 encoded. Valid characters are <ulink
-        variable definitions. The parser strips leading and trailing whitespace from the values of assignments, unless
+        url="https://www.unicode.org/glossary/#unicode_scalar_value">unicode scalar values</ulink> other than <ulink
-        you use double quotes (").</para>
+        url="https://www.unicode.org/glossary/#noncharacter">noncharacters</ulink>, U+0000 NUL, and U+FEFF <ulink
+        url="https://www.unicode.org/glossary/#byte_order_mark">byte order mark</ulink>. Control codes other than NUL
+        are allowed.</para>
 
-        <para><ulink url="https://en.wikipedia.org/wiki/Escape_sequences_in_C#Table_of_escape_sequences">C escapes</ulink>
+        <para>In the file, an unquoted value after the <literal>=</literal> is parsed with the same backslash-escape
-        are supported, but not
+        rules as <ulink
-        <ulink url="https://en.wikipedia.org/wiki/Control_character#In_ASCII">most control characters</ulink>.
+        url="https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_01">unquoted
-        <literal>\t</literal> and <literal>\n</literal> can be used to insert tabs and newlines within
+        text</ulink> in a POSIX shell, but unlike in a shell, interior whitespace is preserved and quotes after the
-        <varname>EnvironmentFile=</varname>.</para>
+        first non-whitespace character are preserved. Leading and trailing whitespace (space, tab, carriage return) is
+        discarded, but interior whitespace within the line is preserved verbatim. A line ending with a backslash will be
+        continued to the following one, with the newline itself discarded. A backslash
+        <literal>\</literal> followed by any character other than newline will preserve the following character, so that
+        <literal>\\</literal> will become the value <literal>\</literal>.</para>
+
+        <para>In the file, a <literal>'</literal>-quoted value after the <literal>=</literal> can span multiple lines
+        and contain any character verbatim other than single quote, like <ulink
+        url="https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_02">single-quoted
+        text</ulink> in a POSIX shell. No backslash-escape sequences are recognized. Leading and trailing whitespace
+        outside of the single quotes is discarded.</para>
+
+        <para>In the file, a <literal>"</literal>-quoted value after the <literal>=</literal> can span multiple lines,
+        and the same escape sequences are recognized as in <ulink
+        url="https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_03">double-quoted
+        text</ulink> of a POSIX shell. Backslash (<literal>\</literal>) followed by any of <literal>"\`$</literal> will
+        preserve that character. A backslash followed by newline is a line continuation, and the newline itself is
+        discarded. A backslash followed by any other character is ignored; both the backslash and the following
+        character are preserved verbatim. Leading and trailing whitespace outside of the double quotes is
+        discarded.</para>
 
         <para>The argument passed should be an absolute filename or wildcard expression, optionally prefixed with
         <literal>-</literal>, which indicates that if the file does not exist, it will not be read and no error or
@@ -2557,12 +2578,6 @@ SystemCallErrorNumber=EPERM</programlisting>
         <para>Variables set for invoked processes due to this setting are subject to being overridden by those
         configured with <varname>Environment=</varname> or <varname>EnvironmentFile=</varname>.</para>
 
-        <para><ulink url="https://en.wikipedia.org/wiki/Escape_sequences_in_C#Table_of_escape_sequences">C escapes</ulink>
-        are supported, but not
-        <ulink url="https://en.wikipedia.org/wiki/Control_character#In_ASCII">most control characters</ulink>.
-        <literal>\t</literal> and <literal>\n</literal> can be used to insert tabs and newlines within
-        <varname>EnvironmentFile=</varname>.</para>
-
         <para>Example:
         <programlisting>PassEnvironment=VAR1 VAR2 VAR3</programlisting>
         passes three variables <literal>VAR1</literal>,

src/core/execute.c

@@ -5501,20 +5501,18 @@ static int exec_context_named_iofds(
         return targets == 0 ? 0 : -ENOENT;
 }
 
-static int exec_context_load_environment(const Unit *unit, const ExecContext *c, char ***l) {
+static int exec_context_load_environment(const Unit *unit, const ExecContext *c, char ***ret) {
-        char **i, **r = NULL;
+        _cleanup_strv_free_ char **v = NULL;
+        char **i;
+        int r;
 
         assert(c);
-        assert(l);
+        assert(ret);
 
         STRV_FOREACH(i, c->environment_files) {
-                char *fn;
-                int k;
-                bool ignore = false;
-                char **p;
                 _cleanup_globfree_ glob_t pglob = {};
-
+                bool ignore = false;
-                fn = *i;
+                char *fn = *i;
 
                 if (fn[0] == '-') {
                         ignore = true;
@@ -5524,33 +5522,30 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c,
                 if (!path_is_absolute(fn)) {
                         if (ignore)
                                 continue;
-
-                        strv_free(r);
                         return -EINVAL;
                 }
 
                 /* Filename supports globbing, take all matching files */
-                k = safe_glob(fn, 0, &pglob);
+                r = safe_glob(fn, 0, &pglob);
-                if (k < 0) {
+                if (r < 0) {
                         if (ignore)
                                 continue;
-
+                        return r;
-                        strv_free(r);
-                        return k;
                 }
 
                 /* When we don't match anything, -ENOENT should be returned */
                 assert(pglob.gl_pathc > 0);
 
                 for (unsigned n = 0; n < pglob.gl_pathc; n++) {
-                        k = load_env_file(NULL, pglob.gl_pathv[n], &p);
+                        _cleanup_strv_free_ char **p = NULL;
-                        if (k < 0) {
+
+                        r = load_env_file(NULL, pglob.gl_pathv[n], &p);
+                        if (r < 0) {
                                 if (ignore)
                                         continue;
-
+                                return r;
-                                strv_free(r);
-                                return k;
                         }
+
                         /* Log invalid environment variables with filename */
                         if (p) {
                                 InvalidEnvInfo info = {
@@ -5561,23 +5556,19 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c,
                                 p = strv_env_clean_with_callback(p, invalid_env, &info);
                         }
 
-                        if (!r)
+                        if (!v)
-                                r = p;
+                                v = TAKE_PTR(p);
                         else {
-                                char **m;
+                                char **m = strv_env_merge(v, p);
-
-                                m = strv_env_merge(r, p);
-                                strv_free(r);
-                                strv_free(p);
                                 if (!m)
                                         return -ENOMEM;
 
-                                r = m;
+                                strv_free_and_replace(v, m);
                         }
                 }
         }
 
-        *l = r;
+        *ret = TAKE_PTR(v);
 
         return 0;
 }

src/portable/portable.c

@@ -253,6 +253,7 @@ static int extract_now(
 
                 FOREACH_DIRENT(de, d, return log_debug_errno(errno, "Failed to read directory: %m")) {
                         _cleanup_(portable_metadata_unrefp) PortableMetadata *m = NULL;
+                        _cleanup_(mac_selinux_freep) char *con = NULL;
                         _cleanup_close_ int fd = -1;
 
                         if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY))
@@ -274,8 +275,6 @@ static int extract_now(
                                 continue;
                         }
 
-                        if (socket_fd >= 0) {
-                                _cleanup_(mac_selinux_freep) char *con = NULL;
 #if HAVE_SELINUX
                         /* The units will be copied on the host's filesystem, so if they had a SELinux label
                          * we have to preserve it. Copy it out so that it can be applied later. */
@@ -284,6 +283,8 @@ static int extract_now(
                         if (r < 0 && errno != ENODATA)
                                 log_debug_errno(errno, "Failed to get SELinux file context from '%s', ignoring: %m", de->d_name);
 #endif
+
+                        if (socket_fd >= 0) {
                                 struct iovec iov[] = {
                                         IOVEC_MAKE_STRING(de->d_name),
                                         IOVEC_MAKE((char *)"\0", sizeof(char)),
@@ -295,7 +296,7 @@ static int extract_now(
                                         return log_debug_errno(r, "Failed to send unit metadata to parent: %m");
                         }
 
-                        m = portable_metadata_new(de->d_name, NULL, NULL, fd);
+                        m = portable_metadata_new(de->d_name, where, con, fd);
                         if (!m)
                                 return -ENOMEM;
                         fd = -1;
@@ -336,10 +337,16 @@ static int portable_extract_by_path(
 
         r = loop_device_make_by_path(path, O_RDONLY, LO_FLAGS_PARTSCAN, &d);
         if (r == -EISDIR) {
+                _cleanup_free_ char *image_name = NULL;
+
                 /* We can't turn this into a loop-back block device, and this returns EISDIR? Then this is a directory
                  * tree and not a raw device. It's easy then. */
 
-                r = extract_now(path, matches, NULL, path_is_extension, -1, &os_release, &unit_files);
+                r = path_extract_filename(path, &image_name);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to extract image name from path '%s': %m", path);
+
+                r = extract_now(path, matches, image_name, path_is_extension, -1, &os_release, &unit_files);
                 if (r < 0)
                         return r;
 
@@ -872,6 +879,10 @@ static const char *root_setting_from_image(ImageType type) {
         return IN_SET(type, IMAGE_DIRECTORY, IMAGE_SUBVOLUME) ? "RootDirectory=" : "RootImage=";
 }
 
+static const char *extension_setting_from_image(ImageType type) {
+        return IN_SET(type, IMAGE_DIRECTORY, IMAGE_SUBVOLUME) ? "ExtensionDirectories=" : "ExtensionImages=";
+}
+
 static int make_marker_text(const char *image_path, OrderedHashmap *extension_images, char **ret_text) {
         _cleanup_free_ char *text = NULL, *escaped_image_path = NULL;
         Image *ext;
@@ -918,7 +929,6 @@ static int install_chroot_dropin(
                 size_t *n_changes) {
 
         _cleanup_free_ char *text = NULL, *dropin = NULL;
-        Image *ext;
         int r;
 
         assert(image_path);
@@ -936,6 +946,7 @@ static int install_chroot_dropin(
         if (endswith(m->name, ".service")) {
                 const char *os_release_source, *root_type;
                 _cleanup_free_ char *base_name = NULL;
+                Image *ext;
 
                 root_type = root_setting_from_image(type);
 
@@ -962,7 +973,7 @@ static int install_chroot_dropin(
 
                 if (m->image_path && !path_equal(m->image_path, image_path))
                         ORDERED_HASHMAP_FOREACH(ext, extension_images)
-                                if (!strextend(&text, "ExtensionImages=", ext->path, "\n"))
+                                if (!strextend(&text, extension_setting_from_image(ext->type), ext->path, "\n"))
                                         return -ENOMEM;
         }
 

src/test/test-env-file.c

@@ -17,7 +17,7 @@
         "e  \\\n"                               \
         "f  \n"                                 \
         "g=g\\ \n"                              \
-        "h=h\n"                                 \
+        "h= ąęół\\ śćńźżµ \n"                   \
         "i=i\\"
 
 #define env_file_2                              \
@@ -26,7 +26,10 @@
 #define env_file_3 \
         "#SPAMD_ARGS=\"-d --socketpath=/var/lib/bulwark/spamd \\\n" \
         "#--nouser-config                                     \\\n" \
-        "normal=line"
+        "normal=line                                          \\\n" \
+        ";normal=ignored                                      \\\n" \
+        "normal_ignored                                       \\\n" \
+        "normal ignored                                       \\\n"
 
 #define env_file_4                              \
         "# Generated\n"                         \
@@ -42,6 +45,15 @@
         "a=\n"                                  \
         "b="
 
+#define env_file_6                              \
+        "a=\\ \\n \\t \\x \\y \\' \n"           \
+        "b= \\$'                  \n"           \
+        "c= ' \\n\\t\\$\\`\\\\\n"               \
+        "'   \n"                                \
+        "d= \" \\n\\t\\$\\`\\\\\n"              \
+        "\"   \n"
+
+
 TEST(load_env_file_1) {
         _cleanup_strv_free_ char **data = NULL;
         int r;
@@ -59,7 +71,7 @@ TEST(load_env_file_1) {
         assert_se(streq(data[1], "b=bc"));
         assert_se(streq(data[2], "d=de  f"));
         assert_se(streq(data[3], "g=g "));
-        assert_se(streq(data[4], "h=h"));
+        assert_se(streq(data[4], "h=ąęół śćńźżµ"));
         assert_se(streq(data[5], "i=i"));
         assert_se(data[6] == NULL);
 }
@@ -133,6 +145,26 @@ TEST(load_env_file_5) {
         assert_se(data[2] == NULL);
 }
 
+TEST(load_env_file_6) {
+        _cleanup_strv_free_ char **data = NULL;
+        int r;
+
+        _cleanup_(unlink_tempfilep) char name[] = "/tmp/test-load-env-file.XXXXXX";
+        _cleanup_close_ int fd;
+
+        fd = mkostemp_safe(name);
+        assert_se(fd >= 0);
+        assert_se(write(fd, env_file_6, strlen(env_file_6)) == strlen(env_file_6));
+
+        r = load_env_file(NULL, name, &data);
+        assert_se(r == 0);
+        assert_se(streq(data[0], "a= n t x y '"));
+        assert_se(streq(data[1], "b=$'"));
+        assert_se(streq(data[2], "c= \\n\\t\\$\\`\\\\\n"));
+        assert_se(streq(data[3], "d= \\n\\t$`\\\n"));
+        assert_se(data[4] == NULL);
+}
+
 TEST(write_and_load_env_file) {
         const char *v;
 

src/test/test-load-fragment.c

@@ -769,6 +769,70 @@ TEST(config_parse_pass_environ) {
         assert_se(streq(passenv[0], "normal_name"));
 }
 
+TEST(config_parse_unit_env_file) {
+        /* int config_parse_unit_env_file(
+                 const char *unit,
+                 const char *filename,
+                 unsigned line,
+                 const char *section,
+                 unsigned section_line,
+                 const char *lvalue,
+                 int ltype,
+                 const char *rvalue,
+                 void *data,
+                 void *userdata) */
+
+        _cleanup_(manager_freep) Manager *m = NULL;
+        Unit *u;
+        _cleanup_strv_free_ char **files = NULL;
+        int r;
+
+        r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m);
+        if (manager_errno_skip_test(r)) {
+                log_notice_errno(r, "Skipping test: manager_new: %m");
+                return;
+        }
+
+        assert_se(r >= 0);
+        assert_se(manager_startup(m, NULL, NULL, NULL) >= 0);
+
+        assert_se(u = unit_new(m, sizeof(Service)));
+        assert_se(unit_add_name(u, "foobar.service") == 0);
+
+        r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1,
+                                      "EnvironmentFile", 0, "not-absolute",
+                                       &files, u);
+        assert_se(r == 0);
+        assert_se(strv_length(files) == 0);
+
+        r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1,
+                                      "EnvironmentFile", 0, "/absolute1",
+                                       &files, u);
+        assert_se(r == 0);
+        assert_se(strv_length(files) == 1);
+
+        r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1,
+                                      "EnvironmentFile", 0, "/absolute2",
+                                       &files, u);
+        assert_se(r == 0);
+        assert_se(strv_length(files) == 2);
+        assert_se(streq(files[0], "/absolute1"));
+        assert_se(streq(files[1], "/absolute2"));
+
+        r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1,
+                                       "EnvironmentFile", 0, "",
+                                       &files, u);
+        assert_se(r == 0);
+        assert_se(strv_isempty(files));
+
+        r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1,
+                                       "EnvironmentFile", 0, "/path/%n.conf",
+                                       &files, u);
+        assert_se(r == 0);
+        assert_se(strv_length(files) == 1);
+        assert_se(streq(files[0], "/path/foobar.service.conf"));
+}
+
 TEST(unit_dump_config_items) {
         unit_dump_config_items(stdout);
 }

test/units/testsuite-29.sh

@@ -103,7 +103,8 @@ portablectl detach --now --runtime --extension /usr/share/app1.raw /usr/share/mi
 
 # portablectl also works with directory paths rather than images
 
-mkdir /tmp/rootdir /tmp/app1 /tmp/overlay /tmp/os-release-fix /tmp/os-release-fix/etc
+mkdir /tmp/rootdir /tmp/app0 /tmp/app1 /tmp/overlay /tmp/os-release-fix /tmp/os-release-fix/etc
+mount /usr/share/app0.raw /tmp/app0
 mount /usr/share/app1.raw /tmp/app1
 mount /usr/share/minimal_0.raw /tmp/rootdir
 
@@ -124,7 +125,16 @@ systemctl is-active app1.service
 portablectl detach --now --runtime overlay app1
 
 umount /tmp/overlay
+
+portablectl "${ARGS[@]}" attach --copy=symlink --now --runtime --extension /tmp/app0 --extension /tmp/app1 /tmp/rootdir app0 app1
+
+systemctl is-active app0.service
+systemctl is-active app1.service
+
+portablectl detach --now --runtime --extension /tmp/app0 --extension /tmp/app1 /tmp/rootdir app0 app1
+
 umount /tmp/rootdir
+umount /tmp/app0
 umount /tmp/app1
 
 echo OK >/testok