Re: [PATCH v3 2/2] util: Remove possible quadratic behaviour from write_remainder()

[Markus Armbruster <armbru@redhat.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

> write_remainder() steps through the buffers in an IO vector writing out
> everything past a certain byte offset.  However, on each iteration it
> rescans the buffer from the beginning to find out where we're up to.  With
> an unfortunate set of write sizes this could lead to quadratic behaviour.
>
> In an even less likely set of circumstances (total vector length > maximum
> size_t) the 'skip' variable could overflow.  This is one factor in a
> longstanding Coverity error we've seen (although I still can't figure out
> the remainder of its complaint).
>
> Rework write_remainder() to always work out our new position in the vector
> relative to our old/current position, rather than starting from the
> beginning each time.  As a bonus this seems to fix the Coverity error.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  util.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/util.c b/util.c
> index 7db7c2e7..87309c51 100644
> --- a/util.c
> +++ b/util.c
> @@ -597,10 +597,15 @@ int write_all_buf(int fd, const void *buf, size_t len)
>  	size_t left = len;
>  
>  	while (left) {
> -		ssize_t rc = write(fd, p, left);
> +		ssize_t rc;
> +
> +		do
> +			rc = write(fd, p, left);
> +		while ((rc < 0) && errno == EINTR);
>  
>  		if (rc < 0)
>  			return -1;
> +
>  		p += rc;
>  		left -= rc;
>  	}

Uh, shouldn't this be squashed into PATCH 1?

> @@ -615,28 +620,30 @@ int write_all_buf(int fd, const void *buf, size_t len)
>   *
>   * Return: 0 on success, -1 on error (with errno set)
>   *
> - * #syscalls write writev
> + * #syscalls writev
>   */
>  int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip)
>  {
> -	size_t offset, i;
> +	size_t i = 0, offset;
>  
> -	while ((i = iov_skip_bytes(iov, iovcnt, skip, &offset)) < iovcnt) {
> +	while ((i += iov_skip_bytes(iov + i, iovcnt - i, skip, &offset)) < iovcnt) {
>  		ssize_t rc;
>  
>  		if (offset) {
> -			rc = write(fd, (char *)iov[i].iov_base + offset,
> -				   iov[i].iov_len - offset);
> -		} else {
> -			rc = writev(fd, &iov[i], iovcnt - i);
> +			/* Write the remainder of the partially written buffer */
> +			if (write_all_buf(fd, (char *)iov[i].iov_base + offset,
> +					  iov[i].iov_len - offset) < 0)
> +				return -1;
> +			i++;
>  		}
>  
> +		/* Write as much of the remaining whole buffers as we can */
> +		rc = writev(fd, &iov[i], iovcnt - i);
>  		if (rc < 0)
>  			return -1;
>  
> -		skip += rc;
> +		skip = rc;
>  	}
> -
>  	return 0;
>  }