Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On Mon, Apr 04, 2016 at 11:28:00AM +0100, Wei Liu wrote: > On Fri, Apr 01, 2016 at 08:26:16PM +0200, Samuel Thibault wrote: > > 7c8f3483 introduced a break within a loop in netfront.c such that > > cons and nr_consumed were no longer always being incremented. The > > offset at cons will be processed multiple times with the break in > > place. > > > > This commit reverts to using the "some" variable in the loop condition, > > but avoids ifdefs for the non-libc case. It also renames it to dobreak > > to make its usage clearer. > > > > Suggested-by: Sarah Newman > > Signed-off-by: Samuel Thibault > > Tested-by: Sarah Newman > > Queued. Thank you both! This is now backported to mini-os.git xen-stable-4.6 branch. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On Fri, Apr 01, 2016 at 08:26:16PM +0200, Samuel Thibault wrote: > 7c8f3483 introduced a break within a loop in netfront.c such that > cons and nr_consumed were no longer always being incremented. The > offset at cons will be processed multiple times with the break in > place. > > This commit reverts to using the "some" variable in the loop condition, > but avoids ifdefs for the non-libc case. It also renames it to dobreak > to make its usage clearer. > > Suggested-by: Sarah Newman > Signed-off-by: Samuel Thibault > Tested-by: Sarah Newman Queued. Thank you both! ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
Samuel Thibault, on Fri 01 Apr 2016 20:17:01 +0200, wrote: > 7c8f3483 introduced a break within a loop in netfront.c such that > cons and nr_consumed were no longer always being incremented. The > offset at cons will be processed multiple times with the break in > place. > > This commit reverts to using the "some" variable in the loop condition, > but avoids ifdefs for the non-libc case. It also renames it to dobreak > to make its usage clearer. > > Signed-off-by: Samuel Thibault > Tested-by: Sarah Newman Please do not commit this one, but the coming mail. Samuel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
7c8f3483 introduced a break within a loop in netfront.c such that
cons and nr_consumed were no longer always being incremented. The
offset at cons will be processed multiple times with the break in
place.
This commit reverts to using the "some" variable in the loop condition,
but avoids ifdefs for the non-libc case. It also renames it to dobreak
to make its usage clearer.
Suggested-by: Sarah Newman
Signed-off-by: Samuel Thibault
Tested-by: Sarah Newman
---
netfront.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/netfront.c b/netfront.c
index 0eca5b5..b8fac62 100644
--- a/netfront.c
+++ b/netfront.c
@@ -97,19 +97,15 @@ void network_rx(struct netfront_dev *dev)
{
RING_IDX rp,cons,req_prod;
int nr_consumed, more, i, notify;
-#ifdef HAVE_LIBC
-int some;
-#endif
+int dobreak;
nr_consumed = 0;
moretodo:
rp = dev->rx.sring->rsp_prod;
rmb(); /* Ensure we see queued responses up to 'rp'. */
-#ifdef HAVE_LIBC
-some = 0;
-#endif
-for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
+dobreak = 0;
+for (cons = dev->rx.rsp_cons; cons != rp && !dobreak; nr_consumed++,
cons++)
{
struct net_buffer* buf;
unsigned char* page;
@@ -134,8 +130,8 @@ moretodo:
len = dev->len;
memcpy(dev->data, page+rx->offset, len);
dev->rlen = len;
- some = 1;
-break;
+ /* No need to receive the rest for now */
+ dobreak = 1;
} else
#endif
dev->netif_rx(page+rx->offset,rx->status);
@@ -144,11 +140,7 @@ moretodo:
dev->rx.rsp_cons=cons;
RING_FINAL_CHECK_FOR_RESPONSES(&dev->rx,more);
-#ifdef HAVE_LIBC
-if(more && !some) goto moretodo;
-#else
-if(more) goto moretodo;
-#endif
+if(more && !dobreak) goto moretodo;
req_prod = dev->rx.req_prod_pvt;
--
2.8.0.rc3
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
7c8f3483 introduced a break within a loop in netfront.c such that
cons and nr_consumed were no longer always being incremented. The
offset at cons will be processed multiple times with the break in
place.
This commit reverts to using the "some" variable in the loop condition,
but avoids ifdefs for the non-libc case. It also renames it to dobreak
to make its usage clearer.
Signed-off-by: Samuel Thibault
Tested-by: Sarah Newman
---
netfront.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/netfront.c b/netfront.c
index 0eca5b5..b8fac62 100644
--- a/netfront.c
+++ b/netfront.c
@@ -97,19 +97,15 @@ void network_rx(struct netfront_dev *dev)
{
RING_IDX rp,cons,req_prod;
int nr_consumed, more, i, notify;
-#ifdef HAVE_LIBC
-int some;
-#endif
+int dobreak;
nr_consumed = 0;
moretodo:
rp = dev->rx.sring->rsp_prod;
rmb(); /* Ensure we see queued responses up to 'rp'. */
-#ifdef HAVE_LIBC
-some = 0;
-#endif
-for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
+dobreak = 0;
+for (cons = dev->rx.rsp_cons; cons != rp && !dobreak; nr_consumed++,
cons++)
{
struct net_buffer* buf;
unsigned char* page;
@@ -134,8 +130,8 @@ moretodo:
len = dev->len;
memcpy(dev->data, page+rx->offset, len);
dev->rlen = len;
- some = 1;
-break;
+ /* No need to receive the rest for now */
+ dobreak = 1;
} else
#endif
dev->netif_rx(page+rx->offset,rx->status);
@@ -144,11 +140,7 @@ moretodo:
dev->rx.rsp_cons=cons;
RING_FINAL_CHECK_FOR_RESPONSES(&dev->rx,more);
-#ifdef HAVE_LIBC
-if(more && !some) goto moretodo;
-#else
-if(more) goto moretodo;
-#endif
+if(more && !dobreak) goto moretodo;
req_prod = dev->rx.req_prod_pvt;
--
2.8.0.rc3
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On Sat, Mar 26, 2016 at 01:53:28PM -0700, Sarah Newman wrote: > On 03/25/2016 11:33 AM, Samuel Thibault wrote: > >> On Wed, Mar 23, 2016 at 02:26:51PM -0700, Sarah Newman wrote: > >>> 7c8f3483 introduced a break within a loop in netfront.c such that > >>> cons and nr_consumed were no longer always being incremented. The > >>> offset at cons will be processed multiple times with the break in > >>> place. > >>> > >>> Remove the break and re-add "some !=0" in the loop for HAVE_LIBC. > > > > Mmm, right. > > > > That ifdef makes things even more difficult to understand then. That > > however makes me think: how about the attached patch, which actually > > simplifies the rest. > > > > Thanks! > > Samuel > > > > Does anything else need to happen for this patch to be used with xen master > and stable 4.6? > I take it that you and Samuel have reached an agreement on how to proceed? Can either of you submit a proper final patch to xen-devel and minios-devel and CC me please? I will see to its backport. Note that we are near 4.7 freeze at the moment, so it might take some time for me to get to it. But I don't think I will miss it because I want the patch to be in for 4.7. Wei. > Thanks, Sarah ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On 03/25/2016 11:33 AM, Samuel Thibault wrote: >> On Wed, Mar 23, 2016 at 02:26:51PM -0700, Sarah Newman wrote: >>> 7c8f3483 introduced a break within a loop in netfront.c such that >>> cons and nr_consumed were no longer always being incremented. The >>> offset at cons will be processed multiple times with the break in >>> place. >>> >>> Remove the break and re-add "some !=0" in the loop for HAVE_LIBC. > > Mmm, right. > > That ifdef makes things even more difficult to understand then. That > however makes me think: how about the attached patch, which actually > simplifies the rest. > > Thanks! > Samuel > Does anything else need to happen for this patch to be used with xen master and stable 4.6? Thanks, Sarah ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On 03/25/2016 12:32 PM, Samuel Thibault wrote: > Sarah Newman, on Fri 25 Mar 2016 12:19:23 -0700, wrote: >> I have no objections to backing out additional changes made in 7c8f3483, > > ? My patch doesn't really back out more than what you proposed actually. It also backs out the #ifdef's on HAVE_LIBC, but yes it's functionally equivalent. --Sarah ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
Sarah Newman, on Fri 25 Mar 2016 12:19:23 -0700, wrote: > I have no objections to backing out additional changes made in 7c8f3483, ? My patch doesn't really back out more than what you proposed actually. > The patch tests OK with GNT_DEBUG enabled. Good :) Samuel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
On 03/25/2016 11:33 AM, Samuel Thibault wrote: > Wei Liu, on Fri 25 Mar 2016 13:09:07 +, wrote: >> CC Samuel > > Thanks. > >> On Wed, Mar 23, 2016 at 02:26:51PM -0700, Sarah Newman wrote: >>> 7c8f3483 introduced a break within a loop in netfront.c such that >>> cons and nr_consumed were no longer always being incremented. The >>> offset at cons will be processed multiple times with the break in >>> place. >>> >>> Remove the break and re-add "some !=0" in the loop for HAVE_LIBC. > > Mmm, right. > > That ifdef makes things even more difficult to understand then. That > however makes me think: how about the attached patch, which actually > simplifies the rest. > > Thanks! > Samuel > I have no objections to backing out additional changes made in 7c8f3483, but I wasn't the person who submitted that patch. The patch tests OK with GNT_DEBUG enabled. --Sarah ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
Wei Liu, on Fri 25 Mar 2016 13:09:07 +, wrote:
> CC Samuel
Thanks.
> On Wed, Mar 23, 2016 at 02:26:51PM -0700, Sarah Newman wrote:
> > 7c8f3483 introduced a break within a loop in netfront.c such that
> > cons and nr_consumed were no longer always being incremented. The
> > offset at cons will be processed multiple times with the break in
> > place.
> >
> > Remove the break and re-add "some !=0" in the loop for HAVE_LIBC.
Mmm, right.
That ifdef makes things even more difficult to understand then. That
however makes me think: how about the attached patch, which actually
simplifies the rest.
Thanks!
Samuel
netfront: fix off-by-one error introduced in 7c8f3483
7c8f3483 introduced a break within a loop in netfront.c such that
cons and nr_consumed were no longer always being incremented. The
offset at cons will be processed multiple times with the break in
place.
Remove the break and re-add "some !=0" in the loop for HAVE_LIBC,
rename it into dobreak to mitigate confusion.
Signed-off-by: Sarah Newman
Signed-off-by: Samuel Thibault
diff --git a/netfront.c b/netfront.c
index 0eca5b5..b8fac62 100644
--- a/netfront.c
+++ b/netfront.c
@@ -97,19 +97,15 @@ void network_rx(struct netfront_dev *dev)
{
RING_IDX rp,cons,req_prod;
int nr_consumed, more, i, notify;
-#ifdef HAVE_LIBC
-int some;
-#endif
+int dobreak;
nr_consumed = 0;
moretodo:
rp = dev->rx.sring->rsp_prod;
rmb(); /* Ensure we see queued responses up to 'rp'. */
-#ifdef HAVE_LIBC
-some = 0;
-#endif
-for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
+dobreak = 0;
+for (cons = dev->rx.rsp_cons; cons != rp && !dobreak; nr_consumed++,
cons++)
{
struct net_buffer* buf;
unsigned char* page;
@@ -134,8 +130,8 @@ moretodo:
len = dev->len;
memcpy(dev->data, page+rx->offset, len);
dev->rlen = len;
- some = 1;
-break;
+ /* No need to receive the rest for now */
+ dobreak = 1;
} else
#endif
dev->netif_rx(page+rx->offset,rx->status);
@@ -144,11 +140,7 @@ moretodo:
dev->rx.rsp_cons=cons;
RING_FINAL_CHECK_FOR_RESPONSES(&dev->rx,more);
-#ifdef HAVE_LIBC
-if(more && !some) goto moretodo;
-#else
-if(more) goto moretodo;
-#endif
+if(more && !dobreak) goto moretodo;
req_prod = dev->rx.req_prod_pvt;
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
CC Samuel
On Wed, Mar 23, 2016 at 02:26:51PM -0700, Sarah Newman wrote:
> 7c8f3483 introduced a break within a loop in netfront.c such that
> cons and nr_consumed were no longer always being incremented. The
> offset at cons will be processed multiple times with the break in
> place.
>
> Remove the break and re-add "some !=0" in the loop for HAVE_LIBC.
>
> Signed-off-by: Sarah Newman
> ---
> netfront.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/netfront.c b/netfront.c
> index 0eca5b5..557e8c4 100644
> --- a/netfront.c
> +++ b/netfront.c
> @@ -108,8 +108,10 @@ moretodo:
>
> #ifdef HAVE_LIBC
> some = 0;
> -#endif
> +for (cons = dev->rx.rsp_cons; (cons != rp) && !some; nr_consumed++,
> cons++)
> +#else
> for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
> +#endif
> {
> struct net_buffer* buf;
> unsigned char* page;
> @@ -135,7 +137,6 @@ moretodo:
> memcpy(dev->data, page+rx->offset, len);
> dev->rlen = len;
> some = 1;
> -break;
> } else
> #endif
> dev->netif_rx(page+rx->offset,rx->status);
> --
> 1.9.1
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] Mini-OS: netfront: fix off-by-one error introduced in 7c8f3483
7c8f3483 introduced a break within a loop in netfront.c such that
cons and nr_consumed were no longer always being incremented. The
offset at cons will be processed multiple times with the break in
place.
Remove the break and re-add "some !=0" in the loop for HAVE_LIBC.
Signed-off-by: Sarah Newman
---
netfront.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/netfront.c b/netfront.c
index 0eca5b5..557e8c4 100644
--- a/netfront.c
+++ b/netfront.c
@@ -108,8 +108,10 @@ moretodo:
#ifdef HAVE_LIBC
some = 0;
-#endif
+for (cons = dev->rx.rsp_cons; (cons != rp) && !some; nr_consumed++, cons++)
+#else
for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
+#endif
{
struct net_buffer* buf;
unsigned char* page;
@@ -135,7 +137,6 @@ moretodo:
memcpy(dev->data, page+rx->offset, len);
dev->rlen = len;
some = 1;
-break;
} else
#endif
dev->netif_rx(page+rx->offset,rx->status);
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
