Merge pull request #32181 from YHNdnzj/open-file

Some fixes/improvements for OpenFile=

src/core/exec-invoke.c

@@ -3699,11 +3699,12 @@ static int add_shifted_fd(int *fds, size_t fds_size, size_t *n_fds, int *fd) {
 }
 
 static int connect_unix_harder(const ExecContext *c, const ExecParameters *p, const OpenFile *of, int ofd) {
+        static const int socket_types[] = { SOCK_DGRAM, SOCK_STREAM, SOCK_SEQPACKET };
+
         union sockaddr_union addr = {
                 .un.sun_family = AF_UNIX,
         };
         socklen_t sa_len;
-        static const int socket_types[] = { SOCK_DGRAM, SOCK_STREAM, SOCK_SEQPACKET };
         int r;
 
         assert(c);
@@ -3713,43 +3714,35 @@ static int connect_unix_harder(const ExecContext *c, const ExecParameters *p, co
 
         r = sockaddr_un_set_path(&addr.un, FORMAT_PROC_FD_PATH(ofd));
         if (r < 0)
-                return log_exec_error_errno(c, p, r, "Failed to set sockaddr for %s: %m", of->path);
-
+                return log_exec_error_errno(c, p, r, "Failed to set sockaddr for '%s': %m", of->path);
         sa_len = r;
 
-        for (size_t i = 0; i < ELEMENTSOF(socket_types); i++) {
+        FOREACH_ARRAY(i, socket_types, ELEMENTSOF(socket_types)) {
                 _cleanup_close_ int fd = -EBADF;
 
-                fd = socket(AF_UNIX, socket_types[i] | SOCK_CLOEXEC, 0);
+                fd = socket(AF_UNIX, *i|SOCK_CLOEXEC, 0);
                 if (fd < 0)
-                        return log_exec_error_errno(c,
-                                                    p,
-                                                    errno,
-                                                    "Failed to create socket for %s: %m",
+                        return log_exec_error_errno(c, p,
+                                                    errno, "Failed to create socket for '%s': %m",
                                                     of->path);
 
                 r = RET_NERRNO(connect(fd, &addr.sa, sa_len));
-                if (r == -EPROTOTYPE)
-                        continue;
-                if (r < 0)
-                        return log_exec_error_errno(c,
-                                                    p,
-                                                    r,
-                                                    "Failed to connect socket for %s: %m",
+                if (r >= 0)
+                        return TAKE_FD(fd);
+                if (r != -EPROTOTYPE)
+                        return log_exec_error_errno(c, p,
+                                                    r, "Failed to connect to socket for '%s': %m",
                                                     of->path);
-
-                return TAKE_FD(fd);
         }
 
-        return log_exec_error_errno(c,
-                                    p,
-                                    SYNTHETIC_ERRNO(EPROTOTYPE), "Failed to connect socket for \"%s\".",
+        return log_exec_error_errno(c, p,
+                                    SYNTHETIC_ERRNO(EPROTOTYPE), "No suitable socket type to connect to socket '%s'.",
                                     of->path);
 }
 
 static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const OpenFile *of) {
-        struct stat st;
         _cleanup_close_ int fd = -EBADF, ofd = -EBADF;
+        struct stat st;
 
         assert(c);
         assert(p);
@@ -3757,10 +3750,10 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const
 
         ofd = open(of->path, O_PATH | O_CLOEXEC);
         if (ofd < 0)
-                return log_exec_error_errno(c, p, errno, "Could not open \"%s\": %m", of->path);
+                return log_exec_error_errno(c, p, errno, "Failed to open '%s' as O_PATH: %m", of->path);
 
         if (fstat(ofd, &st) < 0)
-                return log_exec_error_errno(c, p, errno, "Failed to stat %s: %m", of->path);
+                return log_exec_error_errno(c, p, errno, "Failed to stat '%s': %m", of->path);
 
         if (S_ISSOCK(st.st_mode)) {
                 fd = connect_unix_harder(c, p, of, ofd);
@@ -3768,10 +3761,11 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const
                         return fd;
 
                 if (FLAGS_SET(of->flags, OPENFILE_READ_ONLY) && shutdown(fd, SHUT_WR) < 0)
-                        return log_exec_error_errno(c, p, errno, "Failed to shutdown send for socket %s: %m",
+                        return log_exec_error_errno(c, p,
+                                                    errno, "Failed to shutdown send for socket '%s': %m",
                                                     of->path);
 
-                log_exec_debug(c, p, "socket %s opened (fd=%d)", of->path, fd);
+                log_exec_debug(c, p, "Opened socket '%s' as fd %d.", of->path, fd);
         } else {
                 int flags = FLAGS_SET(of->flags, OPENFILE_READ_ONLY) ? O_RDONLY : O_RDWR;
                 if (FLAGS_SET(of->flags, OPENFILE_APPEND))
@@ -3781,9 +3775,9 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const
 
                 fd = fd_reopen(ofd, flags | O_CLOEXEC);
                 if (fd < 0)
-                        return log_exec_error_errno(c, p, fd, "Failed to open file %s: %m", of->path);
+                        return log_exec_error_errno(c, p, fd, "Failed to reopen file '%s': %m", of->path);
 
-                log_exec_debug(c, p, "file %s opened (fd=%d)", of->path, fd);
+                log_exec_debug(c, p, "Opened file '%s' as fd %d.", of->path, fd);
         }
 
         return TAKE_FD(fd);
@@ -3802,7 +3796,9 @@ static int collect_open_file_fds(const ExecContext *c, ExecParameters *p, size_t
                 fd = get_open_file_fd(c, p, of);
                 if (fd < 0) {
                         if (FLAGS_SET(of->flags, OPENFILE_GRACEFUL)) {
-                                log_exec_debug_errno(c, p, fd, "Failed to get OpenFile= file descriptor for %s, ignoring: %m", of->path);
+                                log_exec_warning_errno(c, p, fd,
+                                                       "Failed to get OpenFile= file descriptor for '%s', ignoring: %m",
+                                                       of->path);
                                 continue;
                         }
 
@@ -3816,9 +3812,7 @@ static int collect_open_file_fds(const ExecContext *c, ExecParameters *p, size_t
                 if (r < 0)
                         return r;
 
-                p->fds[*n_fds] = TAKE_FD(fd);
-
-                (*n_fds)++;
+                p->fds[(*n_fds)++] = TAKE_FD(fd);
         }
 
         return 0;

src/shared/open-file.c

@@ -1,4 +1,3 @@
-
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
 #include <fcntl.h>
@@ -96,7 +95,7 @@ int open_file_to_string(const OpenFile *of, char **ret) {
         assert(of);
         assert(ret);
 
-        s = shell_escape(of->path, ":");
+        s = xescape(of->path, ":");
         if (!s)
                 return -ENOMEM;
 
@@ -122,21 +121,16 @@ int open_file_to_string(const OpenFile *of, char **ret) {
         return 0;
 }
 
-OpenFile *open_file_free(OpenFile *of) {
+OpenFile* open_file_free(OpenFile *of) {
         if (!of)
                 return NULL;
 
         free(of->path);
         free(of->fdname);
+
         return mfree(of);
 }
 
-void open_file_free_many(OpenFile **head) {
-        assert(head);
-
-        LIST_CLEAR(open_files, *head, open_file_free);
-}
-
 static const char * const open_file_flags_table[_OPENFILE_MAX] = {
         [OPENFILE_READ_ONLY] = "read-only",
         [OPENFILE_APPEND]    = "append",

src/shared/open-file.h

@@ -1,8 +1,8 @@
-
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 #pragma once
 
 #include "list.h"
+#include "macro.h"
 
 typedef enum OpenFileFlag {
         OPENFILE_READ_ONLY = 1 << 0,
@@ -27,10 +27,12 @@ int open_file_validate(const OpenFile *of);
 
 int open_file_to_string(const OpenFile *of, char **ret);
 
-OpenFile *open_file_free(OpenFile *of);
+OpenFile* open_file_free(OpenFile *of);
 DEFINE_TRIVIAL_CLEANUP_FUNC(OpenFile*, open_file_free);
 
-void open_file_free_many(OpenFile **head);
+static inline void open_file_free_many(OpenFile **head) {
+        LIST_CLEAR(open_files, *ASSERT_PTR(head), open_file_free);
+}
 
-const char *open_file_flags_to_string(OpenFileFlag t) _const_;
+const char* open_file_flags_to_string(OpenFileFlag t) _const_;
 OpenFileFlag open_file_flags_from_string(const char *t) _pure_;

src/test/test-open-file.c

@@ -172,14 +172,12 @@ TEST(open_file_to_string) {
         assert_se(streq(s, "/proc/1/ns/mnt::read-only"));
 
         s = mfree(s);
-        assert_se(free_and_strdup(&of->path, "/path:with:colon"));
-        assert_se(free_and_strdup(&of->fdname, "path:with:colon"));
+        ASSERT_OK(free_and_strdup(&of->path, "/path:with:colon"));
+        ASSERT_OK(free_and_strdup(&of->fdname, "path:with:colon"));
         of->flags = 0;
 
-        r = open_file_to_string(of, &s);
-
-        assert_se(r >= 0);
-        assert_se(streq(s, "/path\\:with\\:colon"));
+        ASSERT_OK(open_file_to_string(of, &s));
+        ASSERT_STREQ(s, "/path\\x3awith\\x3acolon");
 }
 
 DEFINE_TEST_MAIN(LOG_INFO);

test/TEST-77-OPENFILE/Makefile

@@ -1 +0,0 @@
-../TEST-01-BASIC/Makefile

test/TEST-77-OPENFILE/test.sh

@@ -1,16 +0,0 @@
-#!/usr/bin/env bash
-# SPDX-License-Identifier: LGPL-2.1-or-later
-set -e
-
-TEST_DESCRIPTION="Openfile tests"
-
-# shellcheck source=test/test-functions
-. "${TEST_BASE_DIR:?}/test-functions"
-
-test_append_files() {
-    local workspace="${1:?}"
-    echo "Open" >"$workspace/test-77-open.dat"
-    echo "File" >"$workspace/test-77-file.dat"
-}
-
-do_test "$@"

test/testsuite-23.units/testsuite-23-openfile-server.socket

@@ -1,5 +1,5 @@
 [Unit]
-Description=TEST-77-OPENFILE server socket
+Description=OpenFile= test server socket
 
 [Socket]
 ListenStream=/tmp/test.sock

test/testsuite-23.units/testsuite-23-openfile-server@.service

@@ -1,5 +1,5 @@
 [Unit]
-Description=TEST-77-OPENFILE server
+Description=OpenFile= test server service
 
 [Service]
 ExecStart=echo "Socket"

test/units/testsuite-23-openfile-child.sh

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+set -eux
+set -o pipefail
+
+# shellcheck source=test/units/util.sh
+. "$(dirname "$0")"/util.sh
+
+assert_eq "$LISTEN_FDS" "$1"
+assert_eq "$LISTEN_FDNAMES" "$2"
+
+for ((i = 3; i < 3 + LISTEN_FDS; i++)); do
+    read -r -u "$i" text
+    assert_eq "$text" "${!i}" # Dereference $i to get i'th arg
+done

test/units/testsuite-23.openfile.sh

@@ -0,0 +1,55 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+set -eux
+set -o pipefail
+
+# shellcheck source=test/units/util.sh
+. "$(dirname "$0")"/util.sh
+
+at_exit() {
+    set +e
+
+    rm -rf /tmp/test-open-file/
+}
+
+trap at_exit EXIT
+
+systemctl log-level debug
+
+# Existing files
+
+mkdir /tmp/test-open-file
+echo "Open" >'/tmp/test-open-file/open.txt'
+echo "File" >'/tmp/test-open-file/file:colon.txt'
+
+systemd-run -p DynamicUser=yes -p EnvironmentFile=-/usr/lib/systemd/systemd-asan-env \
+            -p OpenFile='/tmp/test-open-file/open.txt::read-only' \
+            -p OpenFile='/tmp/test-open-file/file\x3Acolon.txt:colon' \
+            -p RemainAfterExit=yes \
+            --unit=test-23-openfile-existing.service \
+            --service-type=oneshot \
+            /usr/lib/systemd/tests/testdata/units/testsuite-23-openfile-child.sh 2 "open.txt:colon" "Open" "File"
+
+cmp <(systemctl show -p OpenFile test-23-openfile-existing.service) <<EOF
+OpenFile=/tmp/test-open-file/open.txt::read-only
+OpenFile=/tmp/test-open-file/file\\x3acolon.txt:colon
+EOF
+
+systemctl stop test-23-openfile-existing.service
+
+# Sockets
+
+systemctl start testsuite-23-openfile-server.socket
+
+systemd-run -p OpenFile=/tmp/test.sock:socket:read-only \
+            --wait \
+            /usr/lib/systemd/tests/testdata/units/testsuite-23-openfile-child.sh 1 "socket" "Socket"
+
+systemctl stop testsuite-23-openfile-server.socket
+
+# Ignore when missing
+
+assert_rc 202 systemd-run -p OpenFile=/run/missing/foo:missing-file:read-only --wait true
+assert_rc 0 systemd-run -p OpenFile=/run/missing/foo:missing-file:read-only,graceful --wait true
+
+systemctl log-level info

test/units/testsuite-77-client.sh

@@ -1,14 +0,0 @@
-#!/usr/bin/env bash
-# SPDX-License-Identifier: LGPL-2.1-or-later
-set -eux
-set -o pipefail
-
-# shellcheck source=test/units/util.sh
-. "$(dirname "$0")"/util.sh
-
-export SYSTEMD_LOG_LEVEL=debug
-
-assert_eq "$LISTEN_FDS" "1"
-assert_eq "$LISTEN_FDNAMES" "socket"
-read -r -u 3 text
-assert_eq "$text" "Socket"

test/units/testsuite-77-run.sh

@@ -1,14 +0,0 @@
-#!/usr/bin/env bash
-# SPDX-License-Identifier: LGPL-2.1-or-later
-set -eux
-set -o pipefail
-
-# shellcheck source=test/units/util.sh
-. "$(dirname "$0")"/util.sh
-
-export SYSTEMD_LOG_LEVEL=debug
-
-assert_eq "$LISTEN_FDS" "1"
-assert_eq "$LISTEN_FDNAMES" "new-file"
-read -r -u 3 text
-assert_eq "$text" "New"

test/units/testsuite-77.service

@@ -1,10 +0,0 @@
-# SPDX-License-Identifier: LGPL-2.1-or-later
-[Unit]
-Description=TEST-77-OPENFILE
-
-[Service]
-OpenFile=/test-77-open.dat:open:read-only
-OpenFile=/test-77-file.dat
-ExecStartPre=rm -f /failed /testok
-ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh
-Type=oneshot

test/units/testsuite-77.sh

@@ -1,38 +0,0 @@
-#!/usr/bin/env bash
-# SPDX-License-Identifier: LGPL-2.1-or-later
-set -eux
-set -o pipefail
-
-# shellcheck source=test/units/util.sh
-. "$(dirname "$0")"/util.sh
-
-export SYSTEMD_LOG_LEVEL=debug
-
-assert_eq "$LISTEN_FDS" "2"
-assert_eq "$LISTEN_FDNAMES" "open:test-77-file.dat"
-read -r -u 3 text
-assert_eq "$text" "Open"
-read -r -u 4 text
-assert_eq "$text" "File"
-
-# Test for socket
-systemctl start testsuite-77-server.socket
-systemd-run -p OpenFile=/tmp/test.sock:socket:read-only \
-            --wait \
-            --pipe \
-            /usr/lib/systemd/tests/testdata/units/testsuite-77-client.sh
-
-# Tests for D-Bus
-diff <(systemctl show -p OpenFile testsuite-77) - <<EOF
-OpenFile=/test-77-open.dat:open:read-only
-OpenFile=/test-77-file.dat
-EOF
-echo "New" >/test-77-new-file.dat
-systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only "$(dirname "$0")"/testsuite-77-run.sh
-
-assert_rc 202 systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only -p OpenFile=/test-77-mssing-file.dat:missing-file:read-only "$(dirname "$0")"/testsuite-77-run.sh
-
-assert_rc 0 systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only -p OpenFile=/test-77-mssing-file.dat:missing-file:read-only,graceful "$(dirname "$0")"/testsuite-77-run.sh
-
-# End
-touch /testok