Re: [libvirt PATCH v2] iohelper: some refactoring

On Sat, Mar 26, 2022 at 06:06:37PM +0100, Claudio Fontana wrote: > 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 | 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; > +}; It would be much easier to review this patch for correctness if it was split into 2 pieces. First introduce 'struct runIOParams', and second split the runIO method in 2.

> + > +/* 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; > + } I *really* dislike this pattern. It is hard to follow because of the separation of error occurring from error reporting. It will be way too easy to use the wrong return value and end up reporting the wrong error. Also it liable to accidentally overwriting 'errno' values before we get to calling virReportSystemError in the caller.

IMHO virReport*Error calls need to stay in runIOCopy at point the errors occurr. > + 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 > With regards, Daniel