util-lib: optionally, when writing a string to a file, verify string on failure

With this change, the idiom:

    r = write_string_file(p, buf, 0);
    if (r < 0) {
           if (verify_one_line_file(p, buf) > 0)
                   r = 0;
    }

gets reduced to:

    r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);

i.e. when writing the string fails and the new flag
WRITE_STRING_FILE_VERIFY_ON_FAILURE is specified we'll not return a
failure immediately, but check the contents of the file. If it matches
what we wanted to write we suppress the error and exit cleanly.

diff --git a/src/basic/fileio.c b/src/basic/fileio.c
index be6e327..10aacdc 100644 (file)
--- a/src/basic/fileio.c
+++ b/src/basic/fileio.c
@@ -78,6 +78,7 @@ static int write_string_file_atomic(const char *fn, const char *line, bool enfor
 
 int write_string_file(const char *fn, const char *line, WriteStringFileFlags flags) {
         _cleanup_fclose_ FILE *f = NULL;
+        int q, r;
 
         assert(fn);
         assert(line);
@@ -85,30 +86,58 @@ int write_string_file(const char *fn, const char *line, WriteStringFileFlags fla
         if (flags & WRITE_STRING_FILE_ATOMIC) {
                 assert(flags & WRITE_STRING_FILE_CREATE);
 
-                return write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+                r = write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+                if (r < 0)
+                        goto fail;
+
+                return r;
         }
 
         if (flags & WRITE_STRING_FILE_CREATE) {
                 f = fopen(fn, "we");
-                if (!f)
-                        return -errno;
+                if (!f) {
+                        r = -errno;
+                        goto fail;
+                }
         } else {
                 int fd;
 
                 /* We manually build our own version of fopen(..., "we") that
                  * works without O_CREAT */
                 fd = open(fn, O_WRONLY|O_CLOEXEC|O_NOCTTY);
-                if (fd < 0)
-                        return -errno;
+                if (fd < 0) {
+                        r = -errno;
+                        goto fail;
+                }
 
                 f = fdopen(fd, "we");
                 if (!f) {
+                        r = -errno;
                         safe_close(fd);
-                        return -errno;
+                        goto fail;
                 }
         }
 
-        return write_string_stream(f, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        r = write_string_stream(f, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        if (r < 0)
+                goto fail;
+
+        return 0;
+
+fail:
+        if (!(flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE))
+                return r;
+
+        f = safe_fclose(f);
+
+        /* OK, the operation failed, but let's see if the right
+         * contents in place already. If so, eat up the error. */
+
+        q = verify_file(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        if (q <= 0)
+                return r;
+
+        return 0;
 }
 
 int read_one_line_file(const char *fn, char **line) {
@@ -139,15 +168,41 @@ int read_one_line_file(const char *fn, char **line) {
         return 0;
 }
 
-int verify_one_line_file(const char *fn, const char *line) {
-        _cleanup_free_ char *value = NULL;
-        int r;
+int verify_file(const char *fn, const char *blob, bool accept_extra_nl) {
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_free_ char *buf = NULL;
+        size_t l, k;
 
-        r = read_one_line_file(fn, &value);
-        if (r < 0)
-                return r;
+        assert(fn);
+        assert(blob);
+
+        l = strlen(blob);
+
+        if (accept_extra_nl && endswith(blob, "\n"))
+                accept_extra_nl = false;
+
+        buf = malloc(l + accept_extra_nl + 1);
+        if (!buf)
+                return -ENOMEM;
 
-        return streq(value, line);
+        f = fopen(fn, "re");
+        if (!f)
+                return -errno;
+
+        /* We try to read one byte more than we need, so that we know whether we hit eof */
+        errno = 0;
+        k = fread(buf, 1, l + accept_extra_nl + 1, f);
+        if (ferror(f))
+                return errno > 0 ? -errno : -EIO;
+
+        if (k != l && k != l + accept_extra_nl)
+                return 0;
+        if (memcmp(buf, blob, l) != 0)
+                return 0;
+        if (k > l && buf[l] != '\n')
+                return 0;
+
+        return 1;
 }
 
 int read_full_stream(FILE *f, char **contents, size_t *size) {

diff --git a/src/basic/fileio.h b/src/basic/fileio.h
index 5f2c941..95e8698 100644 (file)
--- a/src/basic/fileio.h
+++ b/src/basic/fileio.h
@@ -34,6 +34,7 @@ typedef enum {
         WRITE_STRING_FILE_CREATE = 1,
         WRITE_STRING_FILE_ATOMIC = 2,
         WRITE_STRING_FILE_AVOID_NEWLINE = 4,
+        WRITE_STRING_FILE_VERIFY_ON_FAILURE = 8,
 } WriteStringFileFlags;
 
 int write_string_stream(FILE *f, const char *line, bool enforce_newline);
@@ -43,7 +44,7 @@ int read_one_line_file(const char *fn, char **line);
 int read_full_file(const char *fn, char **contents, size_t *size);
 int read_full_stream(FILE *f, char **contents, size_t *size);
 
-int verify_one_line_file(const char *fn, const char *line);
+int verify_file(const char *fn, const char *blob, bool accept_extra_nl);
 
 int parse_env_file(const char *fname, const char *separator, ...) _sentinel_;
 int load_env_file(FILE *f, const char *fname, const char *separator, char ***l);

diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 00c57b2..07910c2 100644 (file)
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -1859,14 +1859,9 @@ static int link_set_ipv4_forward(Link *link) {
         p = strjoina("/proc/sys/net/ipv4/conf/", link->ifname, "/forwarding");
         v = one_zero(link_ipv4_forward_enabled(link));
 
-        r = write_string_file(p, v, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, v) > 0)
-                        return 0;
-
+        r = write_string_file(p, v, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot configure IPv4 forwarding for interface %s: %m", link->ifname);
-        }
 
         return 0;
 }
@@ -1888,14 +1883,9 @@ static int link_set_ipv6_forward(Link *link) {
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/forwarding");
         v = one_zero(link_ipv6_forward_enabled(link));
 
-        r = write_string_file(p, v, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, v) > 0)
-                        return 0;
-
+        r = write_string_file(p, v, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot configure IPv6 forwarding for interface: %m");
-        }
 
         return 0;
 }
@@ -1917,14 +1907,9 @@ static int link_set_ipv6_privacy_extensions(Link *link) {
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/use_tempaddr");
         xsprintf(buf, "%u", link->network->ipv6_privacy_extensions);
 
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
-
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot configure IPv6 privacy extension for interface: %m");
-        }
 
         return 0;
 }
@@ -1943,14 +1928,9 @@ static int link_set_ipv6_accept_ra(Link *link) {
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/accept_ra");
 
         /* we handle router advertisments ourselves, tell the kernel to GTFO */
-        r = write_string_file(p, "0", 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, "0") > 0)
-                        return 0;
-
+        r = write_string_file(p, "0", WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot disable kernel IPv6 accept_ra for interface: %m");
-        }
 
         return 0;
 }
@@ -1974,14 +1954,9 @@ static int link_set_ipv6_dad_transmits(Link *link) {
 
         xsprintf(buf, "%u", link->network->ipv6_dad_transmits);
 
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
-
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot set IPv6 dad transmits for interface: %m");
-        }
 
         return 0;
 }
@@ -2005,14 +1980,9 @@ static int link_set_ipv6_hop_limit(Link *link) {
 
         xsprintf(buf, "%u", link->network->ipv6_hop_limit);
 
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
-
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot set IPv6 hop limit for interface: %m");
-        }
 
         return 0;
 }

diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c
index e588681..bde3c7c 100644 (file)
--- a/src/test/test-fileio.c
+++ b/src/test/test-fileio.c
@@ -363,6 +363,26 @@ static void test_write_string_file_no_create(void) {
         unlink(fn);
 }
 
+static void test_write_string_file_verify(void) {
+        _cleanup_free_ char *buf = NULL, *buf2 = NULL;
+        int r;
+
+        assert_se(read_one_line_file("/proc/cmdline", &buf) >= 0);
+        assert_se((buf2 = strjoin(buf, "\n", NULL)));
+
+        r = write_string_file("/proc/cmdline", buf, 0);
+        assert_se(r == -EACCES || r == -EIO);
+        r = write_string_file("/proc/cmdline", buf2, 0);
+        assert_se(r == -EACCES || r == -EIO);
+
+        assert_se(write_string_file("/proc/cmdline", buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE) == 0);
+        assert_se(write_string_file("/proc/cmdline", buf2, WRITE_STRING_FILE_VERIFY_ON_FAILURE) == 0);
+
+        r = write_string_file("/proc/cmdline", buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_AVOID_NEWLINE);
+        assert_se(r == -EACCES || r == -EIO);
+        assert_se(write_string_file("/proc/cmdline", buf2, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_AVOID_NEWLINE) == 0);
+}
+
 static void test_load_env_file_pairs(void) {
         char fn[] = "/tmp/test-load_env_file_pairs-XXXXXX";
         int fd;
@@ -419,6 +439,7 @@ int main(int argc, char *argv[]) {
         test_write_string_stream();
         test_write_string_file();
         test_write_string_file_no_create();
+        test_write_string_file_verify();
         test_load_env_file_pairs();
 
         return 0;