slirp: don't zero ti_i since we access it later.

[Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

Posted by Tao Wu via Qemu-devel 6 years, 5 months ago

The current code looks buggy, we zero ti_i while we access
ti_dst/ti_src later.

Signed-off-by: Tao Wu <lepton@google.com>
---
 slirp/tcp_subr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index dc8b4bbb50..da0d53743f 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 		m->m_data += IF_MAXLINKHDR;
 		*mtod(m, struct tcpiphdr *) = *ti;
 		ti = mtod(m, struct tcpiphdr *);
-		memset(&ti->ti, 0, sizeof(ti->ti));
+		switch (af) {
+		case AF_INET:
+		    ti->ti.ti_i4.ih_x1 = 0;
+		    break;
+		case AF_INET6:
+		    ti->ti.ti_i6.ih_x1 = 0;
+		    break;
+		default:
+		    g_assert_not_reached();
+		}
 		flags = TH_ACK;
 	} else {
 		/*
-- 
2.15.0.448.gf294e3d99a-goog

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

Posted by no-reply@patchew.org 6 years, 5 months ago

Hi,

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

Subject: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.
Type: series
Message-id: 20171108225340.10194-1-lepton@google.com

=== 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
 * [new tag]               patchew/20171108225340.10194-1-lepton@google.com -> patchew/20171108225340.10194-1-lepton@google.com
Switched to a new branch 'test'
566ba17a0b slirp: don't zero ti_i since we access it later.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: slirp: don't zero ti_i since we access it later....
ERROR: code indent should never use tabs
#21: FILE: slirp/tcp_subr.c:151:
+^I^Iswitch (af) {$

ERROR: code indent should never use tabs
#22: FILE: slirp/tcp_subr.c:152:
+^I^Icase AF_INET:$

ERROR: code indent should never use tabs
#23: FILE: slirp/tcp_subr.c:153:
+^I^I    ti->ti.ti_i4.ih_x1 = 0;$

ERROR: code indent should never use tabs
#24: FILE: slirp/tcp_subr.c:154:
+^I^I    break;$

ERROR: code indent should never use tabs
#25: FILE: slirp/tcp_subr.c:155:
+^I^Icase AF_INET6:$

ERROR: code indent should never use tabs
#26: FILE: slirp/tcp_subr.c:156:
+^I^I    ti->ti.ti_i6.ih_x1 = 0;$

ERROR: code indent should never use tabs
#27: FILE: slirp/tcp_subr.c:157:
+^I^I    break;$

ERROR: code indent should never use tabs
#28: FILE: slirp/tcp_subr.c:158:
+^I^Idefault:$

ERROR: code indent should never use tabs
#29: FILE: slirp/tcp_subr.c:159:
+^I^I    g_assert_not_reached();$

ERROR: code indent should never use tabs
#30: FILE: slirp/tcp_subr.c:160:
+^I^I}$

total: 10 errors, 0 warnings, 17 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] slirp: don't zero ti_i since we access it later.

Posted by Marc-André Lureau 6 years, 5 months ago

Hi

Adding Guillaume in CC, who wrote that line in commit 98c63057d2144

On Wed, Nov 8, 2017 at 11:53 PM, Tao Wu via Qemu-devel
<qemu-devel@nongnu.org> wrote:
> The current code looks buggy, we zero ti_i while we access
> ti_dst/ti_src later.

Could you described the symptoms and why you fixed it that way?

thanks

>
> Signed-off-by: Tao Wu <lepton@google.com>
> ---
>  slirp/tcp_subr.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index dc8b4bbb50..da0d53743f 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
>                 m->m_data += IF_MAXLINKHDR;
>                 *mtod(m, struct tcpiphdr *) = *ti;
>                 ti = mtod(m, struct tcpiphdr *);
> -               memset(&ti->ti, 0, sizeof(ti->ti));
> +               switch (af) {
> +               case AF_INET:
> +                   ti->ti.ti_i4.ih_x1 = 0;
> +                   break;
> +               case AF_INET6:
> +                   ti->ti.ti_i6.ih_x1 = 0;
> +                   break;
> +               default:
> +                   g_assert_not_reached();
> +               }
>                 flags = TH_ACK;
>         } else {
>                 /*
> --
> 2.15.0.448.gf294e3d99a-goog
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

Posted by Tao Wu via Qemu-devel (吴涛@Eng) 6 years, 5 months ago

Thanks. Actually this is a follow up with my previous effort to fix this
bug.
I was busy on something else and then got lost in that old thread. Now I
just checked some my local patch
to see if they've merged to upstream and then found it out.

This is old thread about this:
http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05544.html


On Thu, Nov 9, 2017 at 2:50 AM, Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> Adding Guillaume in CC, who wrote that line in commit 98c63057d2144
>
> On Wed, Nov 8, 2017 at 11:53 PM, Tao Wu via Qemu-devel
> <qemu-devel@nongnu.org> wrote:
> > The current code looks buggy, we zero ti_i while we access
> > ti_dst/ti_src later.
>
> Could you described the symptoms and why you fixed it that way?
>
> thanks
>
> >
> > Signed-off-by: Tao Wu <lepton@google.com>
> > ---
> >  slirp/tcp_subr.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> > index dc8b4bbb50..da0d53743f 100644
> > --- a/slirp/tcp_subr.c
> > +++ b/slirp/tcp_subr.c
> > @@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti,
> struct mbuf *m,
> >                 m->m_data += IF_MAXLINKHDR;
> >                 *mtod(m, struct tcpiphdr *) = *ti;
> >                 ti = mtod(m, struct tcpiphdr *);
> > -               memset(&ti->ti, 0, sizeof(ti->ti));
> > +               switch (af) {
> > +               case AF_INET:
> > +                   ti->ti.ti_i4.ih_x1 = 0;
> > +                   break;
> > +               case AF_INET6:
> > +                   ti->ti.ti_i6.ih_x1 = 0;
> > +                   break;
> > +               default:
> > +                   g_assert_not_reached();
> > +               }
> >                 flags = TH_ACK;
> >         } else {
> >                 /*
> > --
> > 2.15.0.448.gf294e3d99a-goog
> >
> >
>
>
>
> --
> Marc-André Lureau
>

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

Posted by Samuel Thibault 6 years, 5 months ago

Tao Wu(吴涛@Eng), on jeu. 09 nov. 2017 10:48:27 -0800, wrote:
> Thanks. Actually this is a follow up with my previous effort to fix this bug.
> I was busy on something else and then got lost in that old thread. Now I just
> checked some my local patch
> to see if they've merged to upstream and then found it out.
> 
> This is old thread about this:
> [1]http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05544.html

I knew I had seen that proposal somewhere before :)

Thanks for the patch,
Samuel

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

Posted by Samuel Thibault 6 years, 5 months ago

Hello,

Tao Wu, on mer. 08 nov. 2017 14:53:40 -0800, wrote:
> The current code looks buggy, we zero ti_i while we access
> ti_dst/ti_src later.

Mmm, indeed, looking again at how it was introduced, it was too much.

Samuel