On 09/07/2017 09:44 AM, Nikolay Shirokovskiy wrote:
> saferead is not suitable for direct reads. If file size is not multiple
> of align size then we get EINVAL on the read(2) that is supposed to
> return 0 because read buffer will not be aligned at this point.
>
> Let's not read again after partial read and check that we read
> everything by comparing the number of total bytes read against file size.
> ---
> src/util/iohelper.c | 40 +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/iohelper.c b/src/util/iohelper.c
> index fe15a92..32c5a89 100644
> --- a/src/util/iohelper.c
> +++ b/src/util/iohelper.c
> @@ -78,10 +78,22 @@ runIO(const char *path, int fd, int oflags)
> fdoutname = "stdout";
> /* To make the implementation simpler, we give up on any
> * attempt to use O_DIRECT in a non-trivial manner. */
> - if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) {
> - virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> - _("O_DIRECT read needs entire seekable file"));
> - goto cleanup;
> + if (direct) {
> + if ((end = lseek(fd, 0, SEEK_CUR)) != 0) {
> + virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> + _("O_DIRECT read needs entire seekable file"));
> + goto cleanup;
> + }
> +
> + if ((end = lseek(fd, 0, SEEK_END)) < 0) {
> + virReportSystemError(errno, "%s", _("can not seek file end"));
> + goto cleanup;
> + }
> +
> + if (lseek(fd, 0, SEEK_SET) < 0) {
> + virReportSystemError(errno, "%s", _("can not seek file begin"));
> + goto cleanup;
> + }
This might change the position in the file. I mean, currently there is
no way that the @fd is not set at its beginning. However, I'd like to
have iohelper generic enough so that if a lseek()-ed fd was passed
iohelper reads from there and not reposition itself.
Also, @end is rewritten here twice. It's confusing.
What we can do here is:
cur = lseek(fd, 0, SEEK_CUR);
end = lseek(fd, 0, SEEK_END);
lseek(fd, cur, SEEK_SET);
(with proper error handling obviously)
> }
> break;
> case O_WRONLY:
> @@ -109,7 +121,25 @@ runIO(const char *path, int fd, int oflags)
> while (1) {
> ssize_t got;
>
> - if ((got = saferead(fdin, buf, buflen)) < 0) {
> + /* in case of O_DIRECT we cannot read again to check for EOF
> + * after partial buffer read as it is done in saferead */
> + if (direct && fdin == fd && end - total < buflen) {
> + if (total == end)
> + break;
> +
> + while ((got = read(fd, buf, buflen)) < 0 && errno == EINTR);
In this case, the semicolon should be on a separate line:
while ()
;
> +
> + if (got < end - total) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to read last chunk from %s"),
> + fdinname);
> + goto cleanup;
> + }
> + } else {
> + got = saferead(fdin, buf, buflen);
> + }
> +
> + if (got < 0) {
> virReportSystemError(errno, _("Unable to read %s"), fdinname);
> goto cleanup;
> }
>
Otherwise looking good. Also, ACK to patches 1-3. I'll merge them
shortly so that you can send v2 just for this one.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
