[libvirt PATCH v2] iohelper: some refactoring

Claudio Fontana cfontana at suse.de
Sat Mar 26 17:06:37 UTC 2022


while doing research with alternative implementations of runIO,
it seemed necessary to do some refactoring, in order to separate
parameter setting from the actual copy, so that alternative
copy methods can be researched and hopefully eventually implemented.

No functional changes are expected.

Signed-off-by: Claudio Fontana <cfontana at suse.de>
---
 src/util/iohelper.c | 197 +++++++++++++++++++++++++++-----------------
 1 file changed, 122 insertions(+), 75 deletions(-)

changes v1 -> v2:

* fix missing write


diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 2c91bf4f93..ca569b5ae9 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -45,21 +45,38 @@
 # define O_DIRECT 0
 #endif
 
-static int
-runIO(const char *path, int fd, int oflags)
+struct runIOParams {
+    bool isBlockDev;
+    bool isDirect;
+    bool isWrite;
+    int fdin;
+    const char *fdinname;
+    int fdout;
+    const char *fdoutname;
+};
+
+/* runIOCopy: execute the IO copy based on the passed parameters */
+/**
+ * runIOCopy:
+ * @p: the IO parameters
+ *
+ * Execute the copy based on the passed parameters.
+ *
+ * Returns: size transfered, or < 0 on error.
+ * Errors: -1 = read/write error
+ *         -2 = read error
+ *         -3 = write error
+ *         -4 = truncate error
+ */
+
+static off_t
+runIOCopy(const struct runIOParams p)
 {
     g_autofree void *base = NULL; /* Location to be freed */
     char *buf = NULL; /* Aligned location within base */
     size_t buflen = 1024*1024;
     intptr_t alignMask = 64*1024 - 1;
-    int ret = -1;
-    int fdin, fdout;
-    const char *fdinname, *fdoutname;
-    unsigned long long total = 0;
-    bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0);
-    off_t end = 0;
-    struct stat sb;
-    bool isBlockDev = false;
+    off_t total = 0;
 
 #if WITH_POSIX_MEMALIGN
     if (posix_memalign(&base, alignMask + 1, buflen))
@@ -71,50 +88,6 @@ runIO(const char *path, int fd, int oflags)
     buf = (char *) (((intptr_t) base + alignMask) & ~alignMask);
 #endif
 
-    if (fstat(fd, &sb) < 0) {
-        virReportSystemError(errno,
-                             _("Unable to access file descriptor %d path %s"),
-                             fd, path);
-        goto cleanup;
-    }
-    isBlockDev = S_ISBLK(sb.st_mode);
-
-    switch (oflags & O_ACCMODE) {
-    case O_RDONLY:
-        fdin = fd;
-        fdinname = path;
-        fdout = STDOUT_FILENO;
-        fdoutname = "stdout";
-        /* To make the implementation simpler, we give up on any
-         * attempt to use O_DIRECT in a non-trivial manner.  */
-        if (!isBlockDev && direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) {
-            virReportSystemError(end < 0 ? errno : EINVAL, "%s",
-                                 _("O_DIRECT read needs entire seekable file"));
-            goto cleanup;
-        }
-        break;
-    case O_WRONLY:
-        fdin = STDIN_FILENO;
-        fdinname = "stdin";
-        fdout = fd;
-        fdoutname = path;
-        /* To make the implementation simpler, we give up on any
-         * attempt to use O_DIRECT in a non-trivial manner.  */
-        if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) {
-            virReportSystemError(end < 0 ? errno : EINVAL, "%s",
-                                 _("O_DIRECT write needs empty seekable file"));
-            goto cleanup;
-        }
-        break;
-
-    case O_RDWR:
-    default:
-        virReportSystemError(EINVAL,
-                             _("Unable to process file with flags %d"),
-                             (oflags & O_ACCMODE));
-        goto cleanup;
-    }
-
     while (1) {
         ssize_t got;
 
@@ -124,53 +97,127 @@ runIO(const char *path, int fd, int oflags)
          * writes will be aligned.
          * In other cases using saferead reduces number of syscalls.
          */
-        if (fdin == fd && direct) {
-            if ((got = read(fdin, buf, buflen)) < 0 &&
+        if (!p.isWrite && p.isDirect) {
+            if ((got = read(p.fdin, buf, buflen)) < 0 &&
                 errno == EINTR)
                 continue;
         } else {
-            got = saferead(fdin, buf, buflen);
-        }
-
-        if (got < 0) {
-            virReportSystemError(errno, _("Unable to read %s"), fdinname);
-            goto cleanup;
+            got = saferead(p.fdin, buf, buflen);
         }
+        if (got < 0)
+            return -2;
         if (got == 0)
             break;
 
         total += got;
 
         /* handle last write size align in direct case */
-        if (got < buflen && direct && fdout == fd) {
+        if (got < buflen && p.isDirect && p.isWrite) {
             ssize_t aligned_got = (got + alignMask) & ~alignMask;
 
             memset(buf + got, 0, aligned_got - got);
 
-            if (safewrite(fdout, buf, aligned_got) < 0) {
-                virReportSystemError(errno, _("Unable to write %s"), fdoutname);
-                goto cleanup;
+            if (safewrite(p.fdout, buf, aligned_got) < 0) {
+                return -3;
             }
-
-            if (!isBlockDev && ftruncate(fd, total) < 0) {
-                virReportSystemError(errno, _("Unable to truncate %s"), fdoutname);
-                goto cleanup;
+            if (!p.isBlockDev && ftruncate(p.fdout, total) < 0) {
+                return -4;
             }
-
             break;
         }
 
-        if (safewrite(fdout, buf, got) < 0) {
-            virReportSystemError(errno, _("Unable to write %s"), fdoutname);
+        if (safewrite(p.fdout, buf, got) < 0) {
+            return -3;
+        }
+    }
+    return total;
+}
+
+static int
+runIO(const char *path, int fd, int oflags)
+{
+    int ret = -1;
+    off_t total = 0;
+    struct stat sb;
+    struct runIOParams p;
+
+    if (fstat(fd, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to access file descriptor %d path %s"),
+                             fd, path);
+        goto cleanup;
+    }
+    p.isBlockDev = S_ISBLK(sb.st_mode);
+    p.isDirect = O_DIRECT && (oflags & O_DIRECT);
+
+    switch (oflags & O_ACCMODE) {
+    case O_RDONLY:
+        p.isWrite = false;
+        p.fdin = fd;
+        p.fdinname = path;
+        p.fdout = STDOUT_FILENO;
+        p.fdoutname = "stdout";
+        break;
+    case O_WRONLY:
+        p.isWrite = true;
+        p.fdin = STDIN_FILENO;
+        p.fdinname = "stdin";
+        p.fdout = fd;
+        p.fdoutname = path;
+        break;
+    case O_RDWR:
+    default:
+        virReportSystemError(EINVAL,
+                             _("Unable to process file with flags %d"),
+                             (oflags & O_ACCMODE));
+        goto cleanup;
+    }
+    /* To make the implementation simpler, we give up on any
+     * attempt to use O_DIRECT in a non-trivial manner.  */
+    if (!p.isBlockDev && p.isDirect) {
+        off_t end;
+        if (p.isWrite) {
+            if ((end = lseek(fd, 0, SEEK_END)) != 0) {
+                virReportSystemError(end < 0 ? errno : EINVAL, "%s",
+                                     _("O_DIRECT write needs empty seekable file"));
+                goto cleanup;
+            }
+        } else if ((end = lseek(fd, 0, SEEK_CUR)) != 0) {
+            virReportSystemError(end < 0 ? errno : EINVAL, "%s",
+                                 _("O_DIRECT read needs entire seekable file"));
             goto cleanup;
         }
     }
 
+    total = runIOCopy(p);
+
+    if (total < 0) {
+        switch (total) {
+        case -1:
+            virReportSystemError(errno, _("Unable to move data from %s to %s"),
+                                 p.fdinname, p.fdoutname);
+            break;
+        case -2:
+            virReportSystemError(errno, _("Unable to read %s"), p.fdinname);
+            break;
+        case -3:
+            virReportSystemError(errno, _("Unable to write %s"), p.fdoutname);
+            break;
+        case -4:
+            virReportSystemError(errno, _("Unable to truncate %s"), p.fdoutname);
+            break;
+        default:
+            virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown runIOCopy error %ld"), total);
+            break;
+        }
+        goto cleanup;
+    }
+
     /* Ensure all data is written */
-    if (virFileDataSync(fdout) < 0) {
+    if (virFileDataSync(p.fdout) < 0) {
         if (errno != EINVAL && errno != EROFS) {
             /* fdatasync() may fail on some special FDs, e.g. pipes */
-            virReportSystemError(errno, _("unable to fsync %s"), fdoutname);
+            virReportSystemError(errno, _("unable to fsync %s"), p.fdoutname);
             goto cleanup;
         }
     }
-- 
2.35.1



More information about the libvir-list mailing list