On 20.09.2017 18:05, Daniel P. Berrange wrote:
On Wed, Sep 20, 2017 at 06:00:59PM +0300, Nikolay Shirokovskiy wrote:
On 20.09.2017 17:48, Daniel P. Berrange wrote:
IOW can simplify your patch to just this I believe:> diff --git a/src/util/iohelper.c b/src/util/iohelper.c index fe15a92e6..5416d4506 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags) while (1) { ssize_t got;
- if ((got = saferead(fdin, buf, buflen)) < 0) { + if ((got = read(fdin, buf, buflen)) < 0) { + if (errno == EINTR) + continue; virReportSystemError(errno, _("Unable to read %s"), fdinname); goto cleanup; }
But we still read half the buffer at the file end and then try misaligned read after that.
No, we shouldn't be mis-aligned. The 'buf' variable we use is aligned correctly. If we read half a buffer, we'll then write that data back to libvirtd. So when we go back into read(), we'll be reading into the start of 'buf' again, so we're still correctly aligned. This is the key difference against saferead() - that tries to fill up the buffer before we can write any data, whereas with this patch we'll always write whatever we read immediately
I've tested this patch and this works. From read(2) EINVAL fd is attached to an object which is unsuitable for reading; or the file was opened with the O_DIRECT flag, and either the address specified in buf, the value specified in count, or the current file off‐ set is not suitably aligned. I thought everything should be aligned. But after giving the above sentence a thought - it is not stated ) So in case of EOF at least position may not be aligned. But in open(2): The O_DIRECT flag may impose alignment restrictions on the length and address of user-space buffers and the file offset of I/Os. In Linux alignment restrictions vary by file system and kernel version and might be absent entirely. However there is currently no file system-independent interface for an application to dis‐ cover these restrictions for a given file or file system. Some file systems provide their own interfaces for doing so, for example the XFS_IOC_DIOINFO operation in xfsctl(3). So finally it is not clear should such EOF read be possible or not. But I vote for the simple approach while it works. Nikolay