[PATCH] aoe: error printed 1 too early

[PATCH] aoe: error printed 1 too early

[Roel Kluin]

with while (i-- > 0); i reaches -1 after the loop, so the test below is printed
one too early: 0 still means success.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index cc25057..eeea477 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -173,7 +173,7 @@ skbfree(struct sk_buff *skb)
 		return;
 	while (atomic_read(&skb_shinfo(skb)->dataref) != 1 && i-- > 0)
 		msleep(Sms);
-	if (i <= 0) {
+	if (i < 0) {
 		printk(KERN_ERR
 			"aoe: %s holds ref: %s\n",
 			skb->dev ? skb->dev->name : "netif",

Re: [PATCH] aoe: error printed 1 too early

[Ed Cashin]

On Wed, Feb 25, 2009 at 02:38:54PM +0100, Roel Kluin wrote:
> with while (i-- > 0); i reaches -1 after the loop, so the test below is printed
> one too early: 0 still means success.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> index cc25057..eeea477 100644
> --- a/drivers/block/aoe/aoedev.c
> +++ b/drivers/block/aoe/aoedev.c
> @@ -173,7 +173,7 @@ skbfree(struct sk_buff *skb)
>  		return;
>  	while (atomic_read(&skb_shinfo(skb)->dataref) != 1 && i-- > 0)
>  		msleep(Sms);
> -	if (i <= 0) {
> +	if (i < 0) {
>  		printk(KERN_ERR
>  			"aoe: %s holds ref: %s\n",
>  			skb->dev ? skb->dev->name : "netif",
> 

You are talking about the case where, when entering the loop on the
"last chance" iteration, the states are,

  atomic_read(&skb_shinfo(skb)->dataref == 1
  i == 0

... in which case, the skb can and should be used but is not used
because after the value of i is tested it becomes -1.

Right?

Sounds like a good catch.  Did you find it because you saw the "cannot
free skb" message?

-- 
  Ed Cashin <ecashin@coraid.com>

Re: [PATCH] aoe: error printed 1 too early

[David Miller]

From: Ed Cashin <ecashin@coraid.com>
Date: Wed, 25 Feb 2009 12:09:02 -0500

> Sounds like a good catch.  Did you find it because you saw the "cannot
> free skb" message?

He's finding these by looking for them in the code, not by
actually triggering the condition.

Re: [PATCH] aoe: error printed 1 too early

[Ed Cashin]

On Wed, Feb 25, 2009 at 02:01:36PM -0800, David Miller wrote:
> From: Ed Cashin <ecashin@coraid.com>
> Date: Wed, 25 Feb 2009 12:09:02 -0500
> 
> > Sounds like a good catch.  Did you find it because you saw the "cannot
> > free skb" message?
> 
> He's finding these by looking for them in the code, not by
> actually triggering the condition.

Thanks.  I was curious.

-- 
  Ed Cashin <ecashin@coraid.com>

Re: [PATCH] aoe: error printed 1 too early

[roel kluin]

2009/2/25 David Miller <davem@davemloft.net>:
> From: Ed Cashin <ecashin@coraid.com>
> Date: Wed, 25 Feb 2009 12:09:02 -0500
>
>> Sounds like a good catch.  Did you find it because you saw the "cannot
>> free skb" message?
>
> He's finding these by looking for them in the code, not by
> actually triggering the condition.
>

Actually I made all functions a single line and then run some
bash scripts that parses pseudocode like:

echo "INFO: \7 reaches \9 - 1 after loop"
bli "	
	if \((${npz}[&|][&|])?
		--($w) $em?[<=] ($K|$w|$d)
		((&&|\|\|)$(nestedcode "()"))?\)
	(\{
		($(nestedcode "{}"))*
		break ;
	\}|
		break ;
	)
	$ccode
	(
		$Q(\7 [=$em]= \9|\7 (>=|<) \9)
	|
		return \7 ;
	)
	"

This gives me a match like:
  while(...) {
    if (--i < 0)
      break;
    ...
  }
  ...
  if (i != 0)
    ...
which is wrong since i reaches -1.

Re: [PATCH] aoe: error printed 1 too early

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Wed, 25 Feb 2009 14:38:54 +0100

> with while (i-- > 0); i reaches -1 after the loop, so the test below is printed
> one too early: 0 still means success.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

Applied, thanks.