Re: [libvirt PATCH] iohelper: some refactoring

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(a)suse.de&gt; --- src/util/iohelper.c | 193 +++++++++++++++++++++++++++----------------- 1 file changed, 118 insertions(+), 75 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 2c91bf4f93..c04c97af27 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 ssize_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; + ssize_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,123 @@ 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; } + } + return total; +} - if (safewrite(fdout, buf, got) < 0) { - virReportSystemError(errno, _("Unable to write %s"), fdoutname); +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; } }