[2/2] slirp: Handle error returns from sosendoob()

sosendoob() can return a failure code, but all its callers ignore it.
This is OK in sbappend(), as the comment there states -- we will try
again later in sowrite(). Add a (void) cast to tell Coverity so.
In sowrite() we do need to check the return value -- we should handle
a write failure in sosendoob() the same way we handle a write failure
for the normal data.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/sbuf.c   |  2 +-
 slirp/socket.c | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

* Peter Maydell (peter.maydell@linaro.org) wrote:
> sosendoob() can return a failure code, but all its callers ignore it.
> This is OK in sbappend(), as the comment there states -- we will try
> again later in sowrite(). Add a (void) cast to tell Coverity so.
> In sowrite() we do need to check the return value -- we should handle
> a write failure in sosendoob() the same way we handle a write failure
> for the normal data.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I think this is OK, I do have one worry, which is perhaps there
were errors previously that would just loose OOB but get silently
ignored that perhaps we survived OK.
There's a comment there about seeing EAGAIN or EINTR in the normal
data path and not erroring;   hopefully we don't in the OOB case?

However, it generally seems to be sane, so:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  slirp/sbuf.c   |  2 +-
>  slirp/socket.c | 22 ++++++++++++++++------
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/slirp/sbuf.c b/slirp/sbuf.c
> index 10119d3..912f235 100644
> --- a/slirp/sbuf.c
> +++ b/slirp/sbuf.c
> @@ -91,7 +91,7 @@ sbappend(struct socket *so, struct mbuf *m)
>  	if (so->so_urgc) {
>  		sbappendsb(&so->so_rcv, m);
>  		m_free(m);
> -		sosendoob(so);
> +		(void)sosendoob(so);
>  		return;
>  	}
>  
> diff --git a/slirp/socket.c b/slirp/socket.c
> index a17caa9..84cf13a 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -404,7 +404,14 @@ sowrite(struct socket *so)
>  	DEBUG_ARG("so = %p", so);
>  
>  	if (so->so_urgc) {
> -		sosendoob(so);
> +		if (sosendoob(so) < so->so_urgc) {
> +			/* Treat a short write as a fatal error too,
> +			 * rather than continuing on and sending the urgent
> +			 * data as if it were non-urgent and leaving the
> +			 * so_urgc count wrong.
> +			 */
> +			goto err_disconnected;
> +		}
>  		if (sb->sb_cc == 0)
>  			return 0;
>  	}
> @@ -448,11 +455,7 @@ sowrite(struct socket *so)
>  		return 0;
>  
>  	if (nn <= 0) {
> -		DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",
> -			so->so_state, errno));
> -		sofcantsendmore(so);
> -		tcp_sockclosed(sototcpcb(so));
> -		return -1;
> +		goto err_disconnected;
>  	}
>  
>  #ifndef HAVE_READV
> @@ -479,6 +482,13 @@ sowrite(struct socket *so)
>  		sofcantsendmore(so);
>  
>  	return nn;
> +
> +err_disconnected:
> +	DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",
> +		    so->so_state, errno));
> +	sofcantsendmore(so);
> +	tcp_sockclosed(sototcpcb(so));
> +	return -1;
>  }
>  
>  /*
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 11 July 2017 at 19:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> sosendoob() can return a failure code, but all its callers ignore it.
>> This is OK in sbappend(), as the comment there states -- we will try
>> again later in sowrite(). Add a (void) cast to tell Coverity so.
>> In sowrite() we do need to check the return value -- we should handle
>> a write failure in sosendoob() the same way we handle a write failure
>> for the normal data.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> I think this is OK, I do have one worry, which is perhaps there
> were errors previously that would just loose OOB but get silently
> ignored that perhaps we survived OK.
> There's a comment there about seeing EAGAIN or EINTR in the normal
> data path and not erroring;   hopefully we don't in the OOB case?

Let's hope :-) This way round at least we'll find out if
we ever do.

> However, it generally seems to be sane, so:
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

thanks
-- PMM

Peter Maydell, on lun. 05 juin 2017 17:19:36 +0100, wrote:
> diff --git a/slirp/socket.c b/slirp/socket.c
> index a17caa9..84cf13a 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -404,7 +404,14 @@ sowrite(struct socket *so)
>  	DEBUG_ARG("so = %p", so);
>  
>  	if (so->so_urgc) {
> -		sosendoob(so);
> +		if (sosendoob(so) < so->so_urgc) {

Mmm, I believe one needs to use a copy of so->so_urgc, since sosendoob()
modifies it in the success case?

Samuel