All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] [media] ngene: remove an unneeded condition
@ 2012-04-20 13:15 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-04-20 13:15 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Oliver Endriss, Ralph Metzler, linux-media, kernel-janitors

"stat" is always zero here.  The condition used to be needed, but we
shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
Interface driver".

This doesn't change how the code works, it's just a bit tidier.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
index f129a93..3985738 100644
--- a/drivers/media/dvb/ngene/ngene-core.c
+++ b/drivers/media/dvb/ngene/ngene-core.c
@@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
 	if (stat < 0)
 		goto fail;
 
-	if (!stat)
-		return stat;
+	return 0;
 
-	/* otherwise error: fall through */
 fail:
 	ngwritel(0, NGENE_INT_ENABLE);
 	free_irq(dev->pci_dev->irq, dev);

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [patch] [media] ngene: remove an unneeded condition
@ 2012-04-20 13:15 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-04-20 13:15 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Oliver Endriss, Ralph Metzler, linux-media, kernel-janitors

"stat" is always zero here.  The condition used to be needed, but we
shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
Interface driver".

This doesn't change how the code works, it's just a bit tidier.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
index f129a93..3985738 100644
--- a/drivers/media/dvb/ngene/ngene-core.c
+++ b/drivers/media/dvb/ngene/ngene-core.c
@@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
 	if (stat < 0)
 		goto fail;
 
-	if (!stat)
-		return stat;
+	return 0;
 
-	/* otherwise error: fall through */
 fail:
 	ngwritel(0, NGENE_INT_ENABLE);
 	free_irq(dev->pci_dev->irq, dev);

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
  2012-04-20 13:15 ` Dan Carpenter
@ 2012-04-28 14:57   ` walter harms
  -1 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2012-04-28 14:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Mauro Carvalho Chehab, Oliver Endriss, Ralph Metzler,
	linux-media, kernel-janitors



Am 20.04.2012 15:15, schrieb Dan Carpenter:
> "stat" is always zero here.  The condition used to be needed, but we
> shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
> Interface driver".
> 
> This doesn't change how the code works, it's just a bit tidier.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
> index f129a93..3985738 100644
> --- a/drivers/media/dvb/ngene/ngene-core.c
> +++ b/drivers/media/dvb/ngene/ngene-core.c
> @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
>  	if (stat < 0)
>  		goto fail;
>  
> -	if (!stat)
> -		return stat;
> +	return 0;
>  
> -	/* otherwise error: fall through */
>  fail:
>  	ngwritel(0, NGENE_INT_ENABLE);
>  	free_irq(dev->pci_dev->irq, dev);

it seems more logical to use the positive exit in this case like:

  if (stat >=0)
	return 0;

instead of jumping over return 0:

just my 2 cents,

re,
 wh



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
@ 2012-04-28 14:57   ` walter harms
  0 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2012-04-28 14:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Mauro Carvalho Chehab, Oliver Endriss, Ralph Metzler,
	linux-media, kernel-janitors



Am 20.04.2012 15:15, schrieb Dan Carpenter:
> "stat" is always zero here.  The condition used to be needed, but we
> shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
> Interface driver".
> 
> This doesn't change how the code works, it's just a bit tidier.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
> index f129a93..3985738 100644
> --- a/drivers/media/dvb/ngene/ngene-core.c
> +++ b/drivers/media/dvb/ngene/ngene-core.c
> @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
>  	if (stat < 0)
>  		goto fail;
>  
> -	if (!stat)
> -		return stat;
> +	return 0;
>  
> -	/* otherwise error: fall through */
>  fail:
>  	ngwritel(0, NGENE_INT_ENABLE);
>  	free_irq(dev->pci_dev->irq, dev);

it seems more logical to use the positive exit in this case like:

  if (stat >=0)
	return 0;

instead of jumping over return 0:

just my 2 cents,

re,
 wh



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
  2012-04-28 14:57   ` walter harms
@ 2012-04-28 15:16     ` Dan Carpenter
  -1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-04-28 15:16 UTC (permalink / raw)
  To: walter harms
  Cc: Mauro Carvalho Chehab, Oliver Endriss, Ralph Metzler,
	linux-media, kernel-janitors

On Sat, Apr 28, 2012 at 04:57:35PM +0200, walter harms wrote:
> 
> 
> Am 20.04.2012 15:15, schrieb Dan Carpenter:
> > "stat" is always zero here.  The condition used to be needed, but we
> > shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
> > Interface driver".
> > 
> > This doesn't change how the code works, it's just a bit tidier.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
> > index f129a93..3985738 100644
> > --- a/drivers/media/dvb/ngene/ngene-core.c
> > +++ b/drivers/media/dvb/ngene/ngene-core.c
> > @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
> >  	if (stat < 0)
> >  		goto fail;
> >  
> > -	if (!stat)
> > -		return stat;
> > +	return 0;
> >  
> > -	/* otherwise error: fall through */
> >  fail:
> >  	ngwritel(0, NGENE_INT_ENABLE);
> >  	free_irq(dev->pci_dev->irq, dev);
> 
> it seems more logical to use the positive exit in this case like:
> 
>   if (stat >=0)
> 	return 0;
> 
> instead of jumping over return 0:
> 

I would say it's more readable to handle the error condition as
a special case instead of handling the normal success case as
special.  It's better to be consistent instead of changing back and
forth:

	if (error condition)
		return ret;
	if (error condition)
		return ret;
	if (success conditi)
		return ret;
	if (error condition)
		return ret;


regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
@ 2012-04-28 15:16     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-04-28 15:16 UTC (permalink / raw)
  To: walter harms
  Cc: Mauro Carvalho Chehab, Oliver Endriss, Ralph Metzler,
	linux-media, kernel-janitors

On Sat, Apr 28, 2012 at 04:57:35PM +0200, walter harms wrote:
> 
> 
> Am 20.04.2012 15:15, schrieb Dan Carpenter:
> > "stat" is always zero here.  The condition used to be needed, but we
> > shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
> > Interface driver".
> > 
> > This doesn't change how the code works, it's just a bit tidier.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
> > index f129a93..3985738 100644
> > --- a/drivers/media/dvb/ngene/ngene-core.c
> > +++ b/drivers/media/dvb/ngene/ngene-core.c
> > @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
> >  	if (stat < 0)
> >  		goto fail;
> >  
> > -	if (!stat)
> > -		return stat;
> > +	return 0;
> >  
> > -	/* otherwise error: fall through */
> >  fail:
> >  	ngwritel(0, NGENE_INT_ENABLE);
> >  	free_irq(dev->pci_dev->irq, dev);
> 
> it seems more logical to use the positive exit in this case like:
> 
>   if (stat >=0)
> 	return 0;
> 
> instead of jumping over return 0:
> 

I would say it's more readable to handle the error condition as
a special case instead of handling the normal success case as
special.  It's better to be consistent instead of changing back and
forth:

	if (error condition)
		return ret;
	if (error condition)
		return ret;
	if (success conditi)
		return ret;
	if (error condition)
		return ret;


regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
  2012-04-20 13:15 ` Dan Carpenter
  (?)
  (?)
@ 2012-04-28 16:23 ` walter harms
  -1 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2012-04-28 16:23 UTC (permalink / raw)
  To: kernel-janitors



Am 28.04.2012 17:16, schrieb Dan Carpenter:
> On Sat, Apr 28, 2012 at 04:57:35PM +0200, walter harms wrote:
>>
>>
>> Am 20.04.2012 15:15, schrieb Dan Carpenter:
>>> "stat" is always zero here.  The condition used to be needed, but we
>>> shifted stuff around in 0f0b270f90 "[media] ngene: CXD2099AR Common
>>> Interface driver".
>>>
>>> This doesn't change how the code works, it's just a bit tidier.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/media/dvb/ngene/ngene-core.c b/drivers/media/dvb/ngene/ngene-core.c
>>> index f129a93..3985738 100644
>>> --- a/drivers/media/dvb/ngene/ngene-core.c
>>> +++ b/drivers/media/dvb/ngene/ngene-core.c
>>> @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
>>>  	if (stat < 0)
>>>  		goto fail;
>>>  
>>> -	if (!stat)
>>> -		return stat;
>>> +	return 0;
>>>  
>>> -	/* otherwise error: fall through */
>>>  fail:
>>>  	ngwritel(0, NGENE_INT_ENABLE);
>>>  	free_irq(dev->pci_dev->irq, dev);
>>
>> it seems more logical to use the positive exit in this case like:
>>
>>   if (stat >=0)
>> 	return 0;
>>
>> instead of jumping over return 0:
>>
> 
> I would say it's more readable to handle the error condition as
> a special case instead of handling the normal success case as
> special.  It's better to be consistent instead of changing back and
> forth:
> 
> 	if (error condition)
> 		return ret;
> 	if (error condition)
> 		return ret;
> 	if (success conditi)
> 		return ret;
> 	if (error condition)
> 		return ret;
> 
> 

i agree but in this special case it looks strange. Would that more
readable ?

if (stat < 0)
{
<errorhandling here>	
 return stat;	
}
	
return 0:

re,
 wh

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [patch] [media] ngene: remove an unneeded condition
  2012-04-20 13:15 ` Dan Carpenter
                   ` (2 preceding siblings ...)
  (?)
@ 2012-04-29 13:46 ` Dan Carpenter
  -1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-04-29 13:46 UTC (permalink / raw)
  To: kernel-janitors

On Sat, Apr 28, 2012 at 06:23:58PM +0200, walter harms wrote:
> i agree but in this special case it looks strange. Would that more
> readable ?
> 
> if (stat < 0)
> {
> <errorhandling here>	
>  return stat;	
> }
> 	
> return 0:
> 

No.  Gotos are perfectly fine for unwinding in the error handling
code.  In this case there are several in a row.  Copy and pasting
the error handling leads to forgot to add a kfree() bugs.

It doesn't look weird at all, it looks like the most normal kind of
error handling there is in the kernel.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-04-29 13:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-20 13:15 [patch] [media] ngene: remove an unneeded condition Dan Carpenter
2012-04-20 13:15 ` Dan Carpenter
2012-04-28 14:57 ` walter harms
2012-04-28 14:57   ` walter harms
2012-04-28 15:16   ` Dan Carpenter
2012-04-28 15:16     ` Dan Carpenter
2012-04-28 16:23 ` walter harms
2012-04-29 13:46 ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.