[libvirt] [PATCH 4/4] iohelper: fix reading with O_DIRECT

[Michal Privoznik]

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