libmisc/write_full.c: Improve write_full()

Documentation:

-  Correct the comment documenting the function:

   write_full() doesn't write "up to" count bytes (which is write(2)'s
   behavior, and exactly what this function is designed to avoid), but
   rather exactly count bytes (on success).

-  While fixing the documentation, take the time to add a man-page-like
   comment as in other APIs.  Especially, since we'll have to document
   a few other changes from this patch, such as the modified return
   values.

-  Partial writes are still possible on error.  It's the caller's
   responsibility to handle that possibility.

API:

-  In write(2), it's useful to know how many bytes were transferred,
   since it can have short writes.  In this API, since it either writes
   it all or fails, that value is useless, and callers only want to know
   if it succeeded or not.  Thus, just return 0 or -1.

Implementation:

-  Use `== -1` instead of `< 0` to check for write(2) syscall errors.
   This is wisdom from Michael Kerrisk.  This convention is useful
   because it more explicitly tells maintainers that the only value
   which can lead to that path is -1.  Otherwise, a maintainer of the
   code might be confused to think that other negative values are
   possible.  Keep it simple.

-  The path under `if (res == 0)` was unreachable, since the loop
   condition `while (count > 0)` precludes that possibility.  Remove the
   dead code.

-  Use a temporary variable of type `const char *` to avoid a cast.

-  Rename `res`, which just holds the result from write(2), to `w`,
   which more clearly shows that it's just a very-short-lived variable
   (by it's one-letter name), and also relates itself more to write(2).
   I find it more readable.

-  Move the definition of `w` to the top of the function.  Now that the
   function is significantly shorter, the lifetime of the variable is
   clearer, and I find it more readable this way.

Use:

-  Also use `== -1` to check errors.

Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/lib/commonio.c b/lib/commonio.c
index 73fdb3a16aa7c74d3b64b39a089ecbe19709271a..9349597ae40a38c05a85fdcde6279a37e792e9b2 100644 (file)
--- a/lib/commonio.c
+++ b/lib/commonio.c
@@ -140,7 +140,7 @@ static int do_lock_file (const char *file, const char *lock, bool log)
        pid = getpid ();
        snprintf (buf, sizeof buf, "%lu", (unsigned long) pid);
        len = (ssize_t) strlen (buf) + 1;
-       if (write_full (fd, buf, (size_t) len) != len) {
+       if (write_full(fd, buf, len) == -1) {
                if (log) {
                        (void) fprintf (shadow_logfd,
                                        "%s: %s file write error: %s\n",

diff --git a/lib/prototypes.h b/lib/prototypes.h
index d20abf3e39e1e7971f7b5b9dfc33f11d7525167b..47ed2ca1cdba7886774890d4f2f66ede86e99c26 100644 (file)
--- a/lib/prototypes.h
+++ b/lib/prototypes.h
@@ -527,7 +527,7 @@ extern unsigned long active_sessions_count(const char *name,
 extern bool valid (const char *, const struct passwd *);
 
 /* write_full.c */
-extern ssize_t write_full(int fd, const void *buf, size_t count);
+extern int write_full(int fd, const void *buf, size_t count);
 
 /* xgetpwnam.c */
 extern /*@null@*/ /*@only@*/struct passwd *xgetpwnam (const char *);

diff --git a/lib/write_full.c b/lib/write_full.c
index cde52fbfd7e0461d4a458769859c8d708672ed80..4ef902cd959475cd7c12c11d3c67660c752dc269 100644 (file)
--- a/lib/write_full.c
+++ b/lib/write_full.c
@@ -1,7 +1,8 @@
 /*
- * SPDX-FileCopyrightText:  2023, Christian Göttsche <cgzones@googlemail.com>
+ * SPDX-FileCopyrightText: 2023, Christian Göttsche <cgzones@googlemail.com>
+ * SPDX-FileCopyrightText: 2023, Alejandro Colomar <alx@kernel.org>
  *
- * SPDX-License-Identifier:  BSD-3-Clause
+ * SPDX-License-Identifier: BSD-3-Clause
  */
 
 
@@ -9,44 +10,68 @@
 
 #ident "$Id$"
 
-#include "prototypes.h"
-
 #include <errno.h>
 #include <unistd.h>
 
+#include "prototypes.h"
+
 
 /*
- * write_full - write entire buffer
+ * SYNOPSIS
+ *     int write_full(int fd, const void *buf, size_t count);
+ *
+ * ARGUMENTS
+ *     fd      File descriptor.
+ *     buf     Source buffer to write(2) into 'fd'.
+ *     count   Size of 'buf'.
+ *
+ * DESCRIPTION
+ *     Write 'count' bytes from the buffer starting at 'buf' to the
+ *     file referred to by 'fd'.
+ *
+ *     This function is similar to write(2), except that it retries
+ *     in case of a short write.
+ *
+ *     Since this function either performs a full write, or fails, the
+ *     return value is simpler than for write(2).
  *
- * Write up to count bytes from the buffer starting at buf to the
- * file referred to by the file descriptor fd.
- * Retry in case of a short write.
+ * RETURN VALUE
+ *     0       On success.
+ *     -1      On error.
  *
- * Returns the number of bytes written on success, -1 on error.
+ * ERRORS
+ *     See write(2).
+ *
+ * CAVEATS
+ *     This function can still perform partial writes: if the function
+ *     fails in the loop after one or more write(2) calls have
+ *     succeeded, it will report a failure, but some data may have been
+ *     written.  In such a case, it's the caller's responsibility to
+ *     make sure that the partial write is not problematic, and
+ *     remediate it if it is --maybe by trying to remove the file--.
  */
-ssize_t write_full(int fd, const void *buf, size_t count) {
-       ssize_t written = 0;
 
-       while (count > 0) {
-               ssize_t res;
 
-               res = write(fd, buf, count);
-               if (res < 0) {
-                       if (errno == EINTR) {
-                               continue;
-                       }
+int
+write_full(int fd, const void *buf, size_t count)
+{
+       ssize_t              w;
+       const unsigned char  *p;
 
-                       return res;
-               }
+       p = buf;
+
+       while (count > 0) {
+               w = write(fd, p, count);
+               if (w == -1) {
+                       if (errno == EINTR)
+                               continue;
 
-               if (res == 0) {
-                       break;
+                       return -1;
                }
 
-               written += res;
-               buf = (const unsigned char*)buf + res;
-               count -= res;
+               p += w;
+               count -= w;
        }
 
-       return written;
+       return 0;
 }

diff --git a/libmisc/copydir.c b/libmisc/copydir.c
index cc1a30d29707563bf21f5c49cb7d75d9e41e7939..bbee719ff26be3b5683f1cca184014caae19b535 100644 (file)
--- a/libmisc/copydir.c
+++ b/libmisc/copydir.c
@@ -816,7 +816,7 @@ static int copy_file (const struct path_info *src, const struct path_info *dst,
                        break;
                }
 
-               if (write_full (ofd, buf, cnt) < 0) {
+               if (write_full(ofd, buf, cnt) == -1) {
                        (void) close (ofd);
                        (void) close (ifd);
                        return -1;

diff --git a/libmisc/failure.c b/libmisc/failure.c
index 6fc575c12336c2844abdf038c21f6b94e08b4901..205eec9cabe048ef8d503753a7d9795f95d5e2b6 100644 (file)
--- a/libmisc/failure.c
+++ b/libmisc/failure.c
@@ -86,7 +86,7 @@ void failure (uid_t uid, const char *tty, struct faillog *fl)
         */
 
        if (   (lseek (fd, offset_uid, SEEK_SET) != offset_uid)
-           || (write_full (fd, fl, sizeof *fl) != (ssize_t) sizeof *fl)
+           || (write_full(fd, fl, sizeof *fl) == -1)
            || (close (fd) != 0)) {
                SYSLOG ((LOG_WARN,
                         "Can't write faillog entry for UID %lu in %s.",
@@ -185,7 +185,7 @@ int failcheck (uid_t uid, struct faillog *fl, bool failed)
                fail.fail_cnt = 0;
 
                if (   (lseek (fd, offset_uid, SEEK_SET) != offset_uid)
-                   || (write_full (fd, &fail, sizeof fail) != (ssize_t) sizeof fail)
+                   || (write_full(fd, &fail, sizeof fail) == -1)
                    || (close (fd) != 0)) {
                        SYSLOG ((LOG_WARN,
                                 "Can't reset faillog entry for UID %lu in %s.",

diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
index ae8c4b93a7064e0d8434a217d8817a988ba89dd5..b4e459e6755cdcce93bc11b433e77f59c66922bb 100644 (file)
--- a/libmisc/idmapping.c
+++ b/libmisc/idmapping.c
@@ -215,7 +215,7 @@ void write_mapping(int proc_dir_fd, int ranges, const struct map_range *mappings
                        log_get_progname(), map_file, strerror(errno));
                exit(EXIT_FAILURE);
        }
-       if (write_full(fd, buf, pos - buf) != (pos - buf)) {
+       if (write_full(fd, buf, pos - buf) == -1) {
                fprintf(log_get_logfd(), _("%s: write to %s failed: %s\n"),
                        log_get_progname(), map_file, strerror(errno));
                exit(EXIT_FAILURE);

diff --git a/libmisc/log.c b/libmisc/log.c
index cc38d6dc5fbe506520c769fca2c7e8f42a9026e8..fb867b274c2951421c0cfc1693996a3fc56467c9 100644 (file)
--- a/libmisc/log.c
+++ b/libmisc/log.c
@@ -82,7 +82,7 @@ void dolastlog (
        strncpy (newlog.ll_host, host, sizeof (newlog.ll_host) - 1);
 #endif
        if (   (lseek (fd, offset, SEEK_SET) != offset)
-           || (write_full (fd, &newlog, sizeof newlog) != (ssize_t) sizeof newlog)
+           || (write_full(fd, &newlog, sizeof newlog) == -1)
            || (close (fd) != 0)) {
                SYSLOG ((LOG_WARN,
                         "Can't write lastlog entry for UID %lu in %s.",

diff --git a/libmisc/utmp.c b/libmisc/utmp.c
index 7c7da6971ab5c884579ebb039ae5f844cae88746..2cccdcd2947c6d413019b568104682043405cf0a 100644 (file)
--- a/libmisc/utmp.c
+++ b/libmisc/utmp.c
@@ -97,7 +97,7 @@ static void failtmp (const char *username, const struct utmp *failent)
         * Append the new failure record and close the log file.
         */
 
-       if (   (write_full (fd, failent, sizeof *failent) != (ssize_t) sizeof *failent)
+       if (   (write_full(fd, failent, sizeof *failent) == -1)
            || (close (fd) != 0)) {
                SYSLOG ((LOG_WARN,
                         "Can't append failure of user %s to %s.",
@@ -194,7 +194,7 @@ static void updwtmp (const char *filename, const struct utmp *ut)
 
        fd = open (filename, O_APPEND | O_WRONLY, 0);
        if (fd >= 0) {
-               write_full (fd, ut, sizeof (*ut));
+               write_full(fd, ut, sizeof(*ut));
                close (fd);
        }
 }

diff --git a/src/login.c b/src/login.c
index b5562923a12734b627a34b9c1461e6a680b61c07..0f308dc10390fffe099ad735c2cfa3cbe493e1a8 100644 (file)
--- a/src/login.c
+++ b/src/login.c
@@ -400,7 +400,7 @@ static void exit_handler (unused int sig)
 
 static void alarm_handler (unused int sig)
 {
-       write_full (STDERR_FILENO, tmsg, strlen (tmsg));
+       write_full(STDERR_FILENO, tmsg, strlen(tmsg));
        signal(SIGALRM, exit_handler);
        alarm(2);
 }

diff --git a/src/su.c b/src/su.c
index 84b9246892c32cc71b38703119200f2db404c051..b03344130df1baac5d6cb76f899f7f5234b02ce8 100644 (file)
--- a/src/su.c
+++ b/src/su.c
@@ -164,9 +164,9 @@ static void kill_child (int unused(s))
 {
        if (0 != pid_child) {
                (void) kill (-pid_child, SIGKILL);
-               (void) write_full (STDERR_FILENO, kill_msg, strlen (kill_msg));
+               (void) write_full(STDERR_FILENO, kill_msg, strlen(kill_msg));
        } else {
-               (void) write_full (STDERR_FILENO, wait_msg, strlen (wait_msg));
+               (void) write_full(STDERR_FILENO, wait_msg, strlen(wait_msg));
        }
        _exit (255);
 }

diff --git a/src/useradd.c b/src/useradd.c
index 5b601e01823fe2b8bfbe8576cc7402a5dd2db3fa..a690992777cbac3e12337834da63fb3dfd1c06ac 100644 (file)
--- a/src/useradd.c
+++ b/src/useradd.c
@@ -2067,7 +2067,7 @@ static void faillog_reset (uid_t uid)
                return;
        }
        if (   (lseek (fd, offset_uid, SEEK_SET) != offset_uid)
-           || (write_full (fd, &fl, sizeof (fl)) != (ssize_t) sizeof (fl))
+           || (write_full(fd, &fl, sizeof (fl)) == -1)
            || (fsync (fd) != 0)) {
                fprintf (stderr,
                         _("%s: failed to reset the faillog entry of UID %lu: %s\n"),

diff --git a/src/usermod.c b/src/usermod.c
index 9e8968025b4deefbc3a936bb99f2b605beac5256..61cd11a8dcee8a884ba7db343ee078f182c20a21 100644 (file)
--- a/src/usermod.c
+++ b/src/usermod.c
@@ -1941,7 +1941,7 @@ static void update_lastlog (void)
            && (read (fd, &ll, sizeof ll) == (ssize_t) sizeof ll)) {
                /* Copy the old entry to its new location */
                if (   (lseek (fd, off_newuid, SEEK_SET) != off_newuid)
-                   || (write_full (fd, &ll, sizeof ll) != (ssize_t) sizeof ll)
+                   || (write_full(fd, &ll, sizeof ll) == -1)
                    || (fsync (fd) != 0)) {
                        fprintf (stderr,
                                 _("%s: failed to copy the lastlog entry of user %lu to user %lu: %s\n"),
@@ -1957,7 +1957,7 @@ static void update_lastlog (void)
                        /* Reset the new uid's lastlog entry */
                        memzero (&ll, sizeof (ll));
                        if (   (lseek (fd, off_newuid, SEEK_SET) != off_newuid)
-                           || (write_full (fd, &ll, sizeof ll) != (ssize_t) sizeof ll)
+                           || (write_full(fd, &ll, sizeof ll) == -1)
                            || (fsync (fd) != 0)) {
                                fprintf (stderr,
                                         _("%s: failed to copy the lastlog entry of user %lu to user %lu: %s\n"),
@@ -2001,7 +2001,7 @@ static void update_faillog (void)
            && (read (fd, &fl, sizeof fl) == (ssize_t) sizeof fl)) {
                /* Copy the old entry to its new location */
                if (   (lseek (fd, off_newuid, SEEK_SET) != off_newuid)
-                   || (write_full (fd, &fl, sizeof fl) != (ssize_t) sizeof fl)
+                   || (write_full(fd, &fl, sizeof fl) == -1)
                    || (fsync (fd) != 0)) {
                        fprintf (stderr,
                                 _("%s: failed to copy the faillog entry of user %lu to user %lu: %s\n"),
@@ -2017,7 +2017,8 @@ static void update_faillog (void)
                        /* Reset the new uid's faillog entry */
                        memzero (&fl, sizeof (fl));
                        if (   (lseek (fd, off_newuid, SEEK_SET) != off_newuid)
-                           || (write_full (fd, &fl, sizeof fl) != (ssize_t) sizeof fl)) {
+                           || (write_full(fd, &fl, sizeof fl) == -1))
+                       {
                                fprintf (stderr,
                                         _("%s: failed to copy the faillog entry of user %lu to user %lu: %s\n"),
                                         Prog, (unsigned long) user_id, (unsigned long) user_newid, strerror (errno));