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

[Qemu-devel] [PATCH v2 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.

Changes v1->v2:
 * since sosendoob() updates so->so_urgc, we need to
   capture the old value to check that the write has
   actually written what it is supposed to.

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 | 52 +++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 18 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-id: 1496679576-14336-2-git-send-email-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 v2 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 | 23 +++++++++++++++++------
 2 files changed, 18 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..ecec029 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -404,7 +404,15 @@ sowrite(struct socket *so)
 	DEBUG_ARG("so = %p", so);
 
 	if (so->so_urgc) {
-		sosendoob(so);
+		uint32_t expected = so->so_urgc;
+		if (sosendoob(so) < expected) {
+			/* 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 +456,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 +483,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 v2 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>
> ---
>  slirp/sbuf.c   |  2 +-
>  slirp/socket.c | 23 +++++++++++++++++------
>  2 files changed, 18 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..ecec029 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -404,7 +404,15 @@ sowrite(struct socket *so)
>  	DEBUG_ARG("so = %p", so);
>  
>  	if (so->so_urgc) {
> -		sosendoob(so);
> +		uint32_t expected = so->so_urgc;
> +		if (sosendoob(so) < expected) {

Oops, yes that is better.

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

> +			/* 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 +456,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 +483,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 v2 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:

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

=== 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
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1500029487-14822-1-git-send-email-peter.maydell@linaro.org -> patchew/1500029487-14822-1-git-send-email-peter.maydell@linaro.org
 * [new tag]         patchew/1500048838-16802-1-git-send-email-clg@kaod.org -> patchew/1500048838-16802-1-git-send-email-clg@kaod.org
Switched to a new branch 'test'
9cb97f6 slirp: Handle error returns from sosendoob()
4b27372 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
#35: FILE: slirp/socket.c:354:
+^I^Iuint32_t urgc = so->so_urgc;$

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

ERROR: line over 90 characters
#70: 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
#70: 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
#26: FILE: slirp/sbuf.c:94:
+^I^I(void)sosendoob(so);$

ERROR: code indent should never use tabs
#39: FILE: slirp/socket.c:407:
+^I^Iuint32_t expected = so->so_urgc;$

ERROR: code indent should never use tabs
#40: FILE: slirp/socket.c:408:
+^I^Iif (sosendoob(so) < expected) {$

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

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

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

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

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

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

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

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

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

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

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

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

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

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

total: 16 errors, 1 warnings, 49 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 v2 2/2] slirp: Handle error returns from sosendoob()

[Samuel Thibault]

Dr. David Alan Gilbert, on ven. 14 juil. 2017 15:24:19 +0100, 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>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Applied to my tree, thanks!

Samuel