[Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[Peter Maydell]

At the moment the slirp sosendoob() function doesn't properly
handle errors from slirp_send(), and its callers don't do
anything with its return value either. (Coverity spots the
latter as CID 1005633.)

This patchset attempts to fix that. In the first patch we
fix sosendoob() itself so that we return errors to the caller
and treat short writes as reducing the amount of urgent data
to be sent. In the second patch we make the call in sowrite()
treat any return form sosendoob() which didn't transfer all
the urgent data as a "disconnect the socket" error in the
same way that it already handles errors for writes of
non-urgent data. The call to sosendoob() in sbappend() can
safely ignore any failures as in this case sowrite() will be
called shortly and will deal with them.

Coding style: I've opted to stick with the tab-indent
that the slirp code uses rather than reindent to QEMU standard.
Happy to go the other way if the maintainer prefers.

Disclaimer: I have not tested these patches very hard and
in particular they deal with error paths and with urgent
data, neither of which I have any test case for...

thanks
-- PMM

Peter Maydell (2):
  slirp: Handle error returns from slirp_send() in sosendoob()
  slirp: Handle error returns from sosendoob()

 slirp/sbuf.c   |  2 +-
 slirp/socket.c | 51 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 35 insertions(+), 18 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] slirp: Handle error returns from slirp_send() in sosendoob()

[Peter Maydell]

The code in sosendoob() assumes that slirp_send() always
succeeds, but it might return an OS error code (for instance
if the other end has disconnected). Catch these and return
the caller either -1 on error or the number of urgent bytes
actually written. (None of the callers check this return
value currently, though.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/socket.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 3b49a69..a17caa9 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -345,33 +345,40 @@ sosendoob(struct socket *so)
 	if (sb->sb_rptr < sb->sb_wptr) {
 		/* We can send it directly */
 		n = slirp_send(so, sb->sb_rptr, so->so_urgc, (MSG_OOB)); /* |MSG_DONTWAIT)); */
-		so->so_urgc -= n;
-
-		DEBUG_MISC((dfd, " --- sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
 	} else {
 		/*
 		 * Since there's no sendv or sendtov like writev,
 		 * we must copy all data to a linear buffer then
 		 * send it all
 		 */
+		uint32_t urgc = so->so_urgc;
 		len = (sb->sb_data + sb->sb_datalen) - sb->sb_rptr;
-		if (len > so->so_urgc) len = so->so_urgc;
+		if (len > urgc) {
+			len = urgc;
+		}
 		memcpy(buff, sb->sb_rptr, len);
-		so->so_urgc -= len;
-		if (so->so_urgc) {
+		urgc -= len;
+		if (urgc) {
 			n = sb->sb_wptr - sb->sb_data;
-			if (n > so->so_urgc) n = so->so_urgc;
+			if (n > urgc) {
+				n = urgc;
+			}
 			memcpy((buff + len), sb->sb_data, n);
-			so->so_urgc -= n;
 			len += n;
 		}
 		n = slirp_send(so, buff, len, (MSG_OOB)); /* |MSG_DONTWAIT)); */
+	}
+
 #ifdef DEBUG
-		if (n != len)
-		   DEBUG_ERROR((dfd, "Didn't send all data urgently XXXXX\n"));
+	if (n != len) {
+		DEBUG_ERROR((dfd, "Didn't send all data urgently XXXXX\n"));
+	}
 #endif
-		DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
+	if (n < 0) {
+		return n;
 	}
+	so->so_urgc -= n;
+	DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
 
 	sb->sb_cc -= n;
 	sb->sb_rptr += n;
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] slirp: Handle error returns from sosendoob()

[Peter Maydell]

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(-)

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

Re: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1496679576-14336-1-git-send-email-peter.maydell@linaro.org
Subject: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9af1ed0 slirp: Handle error returns from sosendoob()
ba36c86 slirp: Handle error returns from slirp_send() in sosendoob()

=== OUTPUT BEGIN ===
Checking PATCH 1/2: slirp: Handle error returns from slirp_send() in sosendoob()...
ERROR: code indent should never use tabs
#33: FILE: slirp/socket.c:354:
+^I^Iuint32_t urgc = so->so_urgc;$

ERROR: code indent should never use tabs
#36: FILE: slirp/socket.c:356:
+^I^Iif (len > urgc) {$

ERROR: code indent should never use tabs
#37: FILE: slirp/socket.c:357:
+^I^I^Ilen = urgc;$

ERROR: code indent should never use tabs
#38: FILE: slirp/socket.c:358:
+^I^I}$

ERROR: code indent should never use tabs
#42: FILE: slirp/socket.c:360:
+^I^Iurgc -= len;$

ERROR: code indent should never use tabs
#43: FILE: slirp/socket.c:361:
+^I^Iif (urgc) {$

ERROR: code indent should never use tabs
#46: FILE: slirp/socket.c:363:
+^I^I^Iif (n > urgc) {$

ERROR: code indent should never use tabs
#47: FILE: slirp/socket.c:364:
+^I^I^I^In = urgc;$

ERROR: code indent should never use tabs
#48: FILE: slirp/socket.c:365:
+^I^I^I}$

ERROR: code indent should never use tabs
#54: FILE: slirp/socket.c:370:
+^I}$

ERROR: code indent should never use tabs
#59: FILE: slirp/socket.c:373:
+^Iif (n != len) {$

ERROR: code indent should never use tabs
#60: FILE: slirp/socket.c:374:
+^I^IDEBUG_ERROR((dfd, "Didn't send all data urgently XXXXX\n"));$

ERROR: code indent should never use tabs
#61: FILE: slirp/socket.c:375:
+^I}$

ERROR: code indent should never use tabs
#64: FILE: slirp/socket.c:377:
+^Iif (n < 0) {$

ERROR: code indent should never use tabs
#65: FILE: slirp/socket.c:378:
+^I^Ireturn n;$

ERROR: code indent should never use tabs
#67: FILE: slirp/socket.c:380:
+^Iso->so_urgc -= n;$

ERROR: line over 90 characters
#68: FILE: slirp/socket.c:381:
+	DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));

ERROR: code indent should never use tabs
#68: FILE: slirp/socket.c:381:
+^IDEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));$

total: 18 errors, 0 warnings, 51 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: slirp: Handle error returns from sosendoob()...
ERROR: code indent should never use tabs
#25: FILE: slirp/sbuf.c:94:
+^I^I(void)sosendoob(so);$

ERROR: code indent should never use tabs
#38: FILE: slirp/socket.c:407:
+^I^Iif (sosendoob(so) < so->so_urgc) {$

ERROR: code indent should never use tabs
#39: FILE: slirp/socket.c:408:
+^I^I^I/* Treat a short write as a fatal error too,$

ERROR: code indent should never use tabs
#40: FILE: slirp/socket.c:409:
+^I^I^I * rather than continuing on and sending the urgent$

ERROR: code indent should never use tabs
#41: FILE: slirp/socket.c:410:
+^I^I^I * data as if it were non-urgent and leaving the$

ERROR: code indent should never use tabs
#42: FILE: slirp/socket.c:411:
+^I^I^I * so_urgc count wrong.$

ERROR: code indent should never use tabs
#43: FILE: slirp/socket.c:412:
+^I^I^I */$

ERROR: code indent should never use tabs
#44: FILE: slirp/socket.c:413:
+^I^I^Igoto err_disconnected;$

ERROR: code indent should never use tabs
#45: FILE: slirp/socket.c:414:
+^I^I}$

ERROR: code indent should never use tabs
#58: FILE: slirp/socket.c:458:
+^I^Igoto err_disconnected;$

WARNING: line over 80 characters
#68: FILE: slirp/socket.c:487:
+	DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",

ERROR: code indent should never use tabs
#68: FILE: slirp/socket.c:487:
+^IDEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",$

ERROR: code indent should never use tabs
#69: FILE: slirp/socket.c:488:
+^I^I    so->so_state, errno));$

ERROR: code indent should never use tabs
#70: FILE: slirp/socket.c:489:
+^Isofcantsendmore(so);$

ERROR: code indent should never use tabs
#71: FILE: slirp/socket.c:490:
+^Itcp_sockclosed(sototcpcb(so));$

ERROR: code indent should never use tabs
#72: FILE: slirp/socket.c:491:
+^Ireturn -1;$

total: 15 errors, 1 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[Peter Maydell]

Ping for review?

thanks
-- PMM

On 5 June 2017 at 17:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> At the moment the slirp sosendoob() function doesn't properly
> handle errors from slirp_send(), and its callers don't do
> anything with its return value either. (Coverity spots the
> latter as CID 1005633.)
>
> This patchset attempts to fix that. In the first patch we
> fix sosendoob() itself so that we return errors to the caller
> and treat short writes as reducing the amount of urgent data
> to be sent. In the second patch we make the call in sowrite()
> treat any return form sosendoob() which didn't transfer all
> the urgent data as a "disconnect the socket" error in the
> same way that it already handles errors for writes of
> non-urgent data. The call to sosendoob() in sbappend() can
> safely ignore any failures as in this case sowrite() will be
> called shortly and will deal with them.
>
> Coding style: I've opted to stick with the tab-indent
> that the slirp code uses rather than reindent to QEMU standard.
> Happy to go the other way if the maintainer prefers.
>
> Disclaimer: I have not tested these patches very hard and
> in particular they deal with error paths and with urgent
> data, neither of which I have any test case for...
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   slirp: Handle error returns from slirp_send() in sosendoob()
>   slirp: Handle error returns from sosendoob()
>
>  slirp/sbuf.c   |  2 +-
>  slirp/socket.c | 51 ++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 35 insertions(+), 18 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[Peter Maydell]

Ping^2 ?

thanks
-- PMM

On 26 June 2017 at 13:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping for review?
>
> thanks
> -- PMM
>
> On 5 June 2017 at 17:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>> At the moment the slirp sosendoob() function doesn't properly
>> handle errors from slirp_send(), and its callers don't do
>> anything with its return value either. (Coverity spots the
>> latter as CID 1005633.)
>>
>> This patchset attempts to fix that. In the first patch we
>> fix sosendoob() itself so that we return errors to the caller
>> and treat short writes as reducing the amount of urgent data
>> to be sent. In the second patch we make the call in sowrite()
>> treat any return form sosendoob() which didn't transfer all
>> the urgent data as a "disconnect the socket" error in the
>> same way that it already handles errors for writes of
>> non-urgent data. The call to sosendoob() in sbappend() can
>> safely ignore any failures as in this case sowrite() will be
>> called shortly and will deal with them.
>>
>> Coding style: I've opted to stick with the tab-indent
>> that the slirp code uses rather than reindent to QEMU standard.
>> Happy to go the other way if the maintainer prefers.
>>
>> Disclaimer: I have not tested these patches very hard and
>> in particular they deal with error paths and with urgent
>> data, neither of which I have any test case for...
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (2):
>>   slirp: Handle error returns from slirp_send() in sosendoob()
>>   slirp: Handle error returns from sosendoob()
>>
>>  slirp/sbuf.c   |  2 +-
>>  slirp/socket.c | 51 ++++++++++++++++++++++++++++++++++-----------------
>>  2 files changed, 35 insertions(+), 18 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()

[Samuel Thibault]

Peter Maydell, on dim. 09 juil. 2017 22:21:01 +0100, wrote:
> Ping^2 ?

I'm sorry I'm still too busy ATM, it's still far in my mbox.

Samuel

Re: [Qemu-devel] [PATCH 1/2] slirp: Handle error returns from slirp_send() in sosendoob()

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The code in sosendoob() assumes that slirp_send() always
> succeeds, but it might return an OS error code (for instance
> if the other end has disconnected). Catch these and return
> the caller either -1 on error or the number of urgent bytes
> actually written. (None of the callers check this return
> value currently, though.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

That looks OK to me; the changes don't change the datastructure
usage at all - it's just the updated or so_urgc if it worked.


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

> ---
>  slirp/socket.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 3b49a69..a17caa9 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -345,33 +345,40 @@ sosendoob(struct socket *so)
>  	if (sb->sb_rptr < sb->sb_wptr) {
>  		/* We can send it directly */
>  		n = slirp_send(so, sb->sb_rptr, so->so_urgc, (MSG_OOB)); /* |MSG_DONTWAIT)); */
> -		so->so_urgc -= n;
> -
> -		DEBUG_MISC((dfd, " --- sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
>  	} else {
>  		/*
>  		 * Since there's no sendv or sendtov like writev,
>  		 * we must copy all data to a linear buffer then
>  		 * send it all
>  		 */
> +		uint32_t urgc = so->so_urgc;
>  		len = (sb->sb_data + sb->sb_datalen) - sb->sb_rptr;
> -		if (len > so->so_urgc) len = so->so_urgc;
> +		if (len > urgc) {
> +			len = urgc;
> +		}
>  		memcpy(buff, sb->sb_rptr, len);
> -		so->so_urgc -= len;
> -		if (so->so_urgc) {
> +		urgc -= len;
> +		if (urgc) {
>  			n = sb->sb_wptr - sb->sb_data;
> -			if (n > so->so_urgc) n = so->so_urgc;
> +			if (n > urgc) {
> +				n = urgc;
> +			}
>  			memcpy((buff + len), sb->sb_data, n);
> -			so->so_urgc -= n;
>  			len += n;
>  		}
>  		n = slirp_send(so, buff, len, (MSG_OOB)); /* |MSG_DONTWAIT)); */
> +	}
> +
>  #ifdef DEBUG
> -		if (n != len)
> -		   DEBUG_ERROR((dfd, "Didn't send all data urgently XXXXX\n"));
> +	if (n != len) {
> +		DEBUG_ERROR((dfd, "Didn't send all data urgently XXXXX\n"));
> +	}
>  #endif
> -		DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
> +	if (n < 0) {
> +		return n;
>  	}
> +	so->so_urgc -= n;
> +	DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));
>  
>  	sb->sb_cc -= n;
>  	sb->sb_rptr += n;
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] slirp: Handle error returns from sosendoob()

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/2] slirp: Handle error returns from sosendoob()

[Peter Maydell]

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

Re: [Qemu-devel] [2/2] slirp: Handle error returns from sosendoob()

[Samuel Thibault]

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

Re: [Qemu-devel] [1/2] slirp: Handle error returns from slirp_send() in sosendoob()

[Samuel Thibault]

Peter Maydell, on lun. 05 juin 2017 17:19:35 +0100, wrote:
> The code in sosendoob() assumes that slirp_send() always
> succeeds, but it might return an OS error code (for instance
> if the other end has disconnected). Catch these and return
> the caller either -1 on error or the number of urgent bytes
> actually written. (None of the callers check this return
> value currently, though.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Applied to my tree, thanks!