Re: [libvirt RFCv6 05/27] virfile: change virFileDiskCopy arguments to extend beyond stdin, stdout

On Thu, May 05, 2022 at 02:52:19PM +0200, Claudio Fontana wrote: > Signed-off-by: Claudio Fontana <cfontana(a)suse.de&gt; > --- > src/util/iohelper.c | 17 +--------- > src/util/virfile.c | 82 ++++++++++++++++++++++++++++++++++----------- > src/util/virfile.h | 2 +- > 3 files changed, 64 insertions(+), 37 deletions(-) > > diff --git a/src/util/iohelper.c b/src/util/iohelper.c > index ce10ccb905..055540c8c4 100644 > --- a/src/util/iohelper.c > +++ b/src/util/iohelper.c > @@ -58,7 +58,6 @@ int > main(int argc, char **argv) > { > const char *path; > - int oflags = -1; > int fd = -1; > > program_name = argv[0]; > @@ -79,25 +78,11 @@ main(int argc, char **argv) > program_name, argv[3]); > exit(EXIT_FAILURE); > } > -#ifdef F_GETFL > - oflags = fcntl(fd, F_GETFL); > -#else > - /* Stupid mingw. */ > - if (fd == STDIN_FILENO) > - oflags = O_RDONLY; > - else if (fd == STDOUT_FILENO) > - oflags = O_WRONLY; > -#endif > - if (oflags < 0) { > - fprintf(stderr, _("%s: unable to determine access mode of fd %d"), > - program_name, fd); > - exit(EXIT_FAILURE); > - } > } else { /* unknown argc pattern */ > usage(EXIT_FAILURE); > } > > - if (fd < 0 || virFileDiskCopy(path, fd, oflags) < 0) > + if (fd < 0 || virFileDiskCopy(fd, path, -1, "stdio") < 0) > goto error; > > return 0; > diff --git a/src/util/virfile.c b/src/util/virfile.c > index 30b0eee074..87d3e53819 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -4669,19 +4669,64 @@ runIOCopy(const struct runIOParams p) > return total; > } > > +/** > + * virFileDiskCopy: run IO to copy data between storage and a pipe or socket. > + * > + * @disk_fd: the already open regular file or block device > + * @disk_path: the pathname corresponding to disk_fd (for error reporting) > + * @remote_fd: the pipe or socket > + * Use -1 to auto-choose between STDIN or STDOUT. > + * @remote_path: the pathname corresponding to remote_fd (for error reporting) > + * > + * Note that the direction of the transfer is detected based on the @disk_fd > + * file access mode (man 2 open). Therefore @disk_fd must be opened with > + * O_RDONLY or O_WRONLY. O_RDWR is not supported. > + * > + * virFileDiskCopy always closes the file descriptor disk_fd, > + * and any error during close(2) is reported and considered a failure. > + * > + * Returns: bytes transferred or < 0 on failure. > + */ > > off_t > -virFileDiskCopy(const char *path, int fd, int oflags) > +virFileDiskCopy(int disk_fd, const char *disk_path, int remote_fd, const char *remote_path) > { > int ret = -1; > off_t total = 0; > struct stat sb; > struct runIOParams p; > + int oflags = -1; > + > +#ifdef F_GETFL > + oflags = fcntl(disk_fd, F_GETFL); > +#else /* !F_GETFL */ > + /* > + * mingw still does not support F_GETFL, and does not seem willing to > + * support it in the future. So we need a hack to get iohelper working, > + * specifically possible only when using stdio. > + */ > + if (disk_fd == STDIN_FILENO) { > + oflags = O_RDONLY; > + } else if (disk_fd == STDOUT_FILENO) { > + oflags = O_WRONLY; > + } else { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("%s: disk_fd must be stdin or stdout, due to lack of F_GETFL (mingw)"), > + __FUNCTION__); > + goto cleanup; > + } > +#endif /* !F_GETFL */ With this issue, and the posix_memalign issue, I'm inclined to say we should just take the easy way out and hide virFileDiskCopy behind #ifndef WIN32, since we don't even need it on WIN32.