[PATCH 1/3] V4L: w9968cf, remove bad usage of ERESTARTSYS

[PATCH 1/3] V4L: w9968cf, remove bad usage of ERESTARTSYS

[Jiri Slaby]

w9968cf, remove bad usage of ERESTARTSYS

down_read_trylock can't be interrupted and so ERESTARTSYS would reach
userspace, which is not permitted. Change it to EAGAIN

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit db38c559d37219c32b179ae005ca7e489336ec94
tree f2d606165e6ef2daa6e1a2860f87521222eeecd2
parent 6e42c2183befe136d85e6a8708ee4eabc543774b
author Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:14:03 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:14:03 +0200

 drivers/media/video/w9968cf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/w9968cf.c b/drivers/media/video/w9968cf.c
index 77599f2..5a1b5f5 100644
--- a/drivers/media/video/w9968cf.c
+++ b/drivers/media/video/w9968cf.c
@@ -2679,7 +2679,7 @@ static int w9968cf_open(struct inode* inode, struct file* filp)
 
 	/* This the only safe way to prevent race conditions with disconnect */
 	if (!down_read_trylock(&w9968cf_disconnect))
-		return -ERESTARTSYS;
+		return -EAGAIN;
 
 	cam = (struct w9968cf_device*)video_get_drvdata(video_devdata(filp));
 

[PATCH 2/3] V4L: zc0301, remove bad usage of ERESTARTSYS

[Jiri Slaby]

zc0301, remove bad usage of ERESTARTSYS

down_read_trylock can't be interrupted and so ERESTARTSYS would reach
userspace, which is not permitted. Change it to EAGAIN

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 235cf594bc65128250632a642f3e9d7e4df4975e
tree 0557746416827daeb4d829610fec2f0c9111a675
parent db38c559d37219c32b179ae005ca7e489336ec94
author Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:15:47 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:15:47 +0200

 drivers/media/video/zc0301/zc0301_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/zc0301/zc0301_core.c b/drivers/media/video/zc0301/zc0301_core.c
index 35138c5..08a93c3 100644
--- a/drivers/media/video/zc0301/zc0301_core.c
+++ b/drivers/media/video/zc0301/zc0301_core.c
@@ -655,7 +655,7 @@ static int zc0301_open(struct inode* inode, struct file* filp)
 	int err = 0;
 
 	if (!down_read_trylock(&zc0301_dev_lock))
-		return -ERESTARTSYS;
+		return -EAGAIN;
 
 	cam = video_get_drvdata(video_devdata(filp));
 

[PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Jiri Slaby]

cinergyT2, remove bad usage of ERESTARTSYS

test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
ERESTARTSYS would reach userspace, which is not permitted. Change it to
EAGAIN

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit b227f5517ddd99f581a9d241c8ba9c50c50fbc3e
tree f469eea4a298db2d4fe27313e48901d74211b2ca
parent 235cf594bc65128250632a642f3e9d7e4df4975e
author Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:24:32 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:24:32 +0200

 drivers/media/dvb/cinergyT2/cinergyT2.c |   38 ++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/media/dvb/cinergyT2/cinergyT2.c b/drivers/media/dvb/cinergyT2/cinergyT2.c
index 154a7ce..a1f6ebd 100644
--- a/drivers/media/dvb/cinergyT2/cinergyT2.c
+++ b/drivers/media/dvb/cinergyT2/cinergyT2.c
@@ -345,7 +345,9 @@ static int cinergyt2_start_feed(struct dvb_demux_feed *dvbdmxfeed)
 	struct dvb_demux *demux = dvbdmxfeed->demux;
 	struct cinergyt2 *cinergyt2 = demux->priv;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	if (cinergyt2->streaming == 0)
@@ -361,7 +363,9 @@ static int cinergyt2_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
 	struct dvb_demux *demux = dvbdmxfeed->demux;
 	struct cinergyt2 *cinergyt2 = demux->priv;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	if (--cinergyt2->streaming == 0)
@@ -481,12 +485,14 @@ static int cinergyt2_open (struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct cinergyt2 *cinergyt2 = dvbdev->priv;
-	int err = -ERESTARTSYS;
+	int err = -EAGAIN;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		goto out;
+	if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
 		goto out;
 
-	if (mutex_lock_interruptible(&cinergyt2->sem))
+	if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
 		goto out_unlock1;
 
 	if ((err = dvb_generic_open(inode, file)))
@@ -550,7 +556,9 @@ static unsigned int cinergyt2_poll (struct file *file, struct poll_table_struct
 	struct cinergyt2 *cinergyt2 = dvbdev->priv;
 	unsigned int mask = 0;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	poll_wait(file, &cinergyt2->poll_wq, wait);
@@ -625,7 +633,9 @@ static int cinergyt2_ioctl (struct inode *inode, struct file *file,
 		if (copy_from_user(&p, (void  __user*) arg, sizeof(p)))
 			return -EFAULT;
 
-		if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+		if (cinergyt2->disconnect_pending)
+			return -EAGAIN;
+		if (mutex_lock_interruptible(&cinergyt2->sem))
 			return -ERESTARTSYS;
 
 		param->cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS;
@@ -996,7 +1006,9 @@ static int cinergyt2_suspend (struct usb_interface *intf, pm_message_t state)
 {
 	struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->wq_sem))
 		return -ERESTARTSYS;
 
 	cinergyt2_suspend_rc(cinergyt2);
@@ -1017,16 +1029,16 @@ static int cinergyt2_resume (struct usb_interface *intf)
 {
 	struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
 	struct dvbt_set_parameters_msg *param = &cinergyt2->param;
-	int err = -ERESTARTSYS;
+	int err = -EAGAIN;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		goto out;
+	if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
 		goto out;
 
-	if (mutex_lock_interruptible(&cinergyt2->sem))
+	if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
 		goto out_unlock1;
 
-	err = 0;
-
 	if (!cinergyt2->sleeping) {
 		cinergyt2_sleep(cinergyt2, 0);
 		cinergyt2_command(cinergyt2, (char *) param, sizeof(*param), NULL, 0);

Re: [PATCH 1/3] V4L: w9968cf, remove bad usage of ERESTARTSYS

[Luca Risolia]

acked-by: Luca Risolia <luca.risolia@studio.unibo.it>

On Monday 08 October 2007 14:40:13 Jiri Slaby wrote:
> w9968cf, remove bad usage of ERESTARTSYS
>
> down_read_trylock can't be interrupted and so ERESTARTSYS would reach
> userspace, which is not permitted. Change it to EAGAIN
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>
> ---
> commit db38c559d37219c32b179ae005ca7e489336ec94
> tree f2d606165e6ef2daa6e1a2860f87521222eeecd2
> parent 6e42c2183befe136d85e6a8708ee4eabc543774b
> author Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:14:03 +0200
> committer Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:14:03 +0200
>
>  drivers/media/video/w9968cf.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/w9968cf.c b/drivers/media/video/w9968cf.c
> index 77599f2..5a1b5f5 100644
> --- a/drivers/media/video/w9968cf.c
> +++ b/drivers/media/video/w9968cf.c
> @@ -2679,7 +2679,7 @@ static int w9968cf_open(struct inode* inode, struct
> file* filp)
>
>  	/* This the only safe way to prevent race conditions with disconnect */
>  	if (!down_read_trylock(&w9968cf_disconnect))
> -		return -ERESTARTSYS;
> +		return -EAGAIN;
>
>  	cam = (struct w9968cf_device*)video_get_drvdata(video_devdata(filp));

Re: [PATCH 2/3] V4L: zc0301, remove bad usage of ERESTARTSYS

[Luca Risolia]

acked-by: Luca Risolia <luca.risolia@studio.unibo.it>


On Monday 08 October 2007 14:40:53 Jiri Slaby wrote:
> zc0301, remove bad usage of ERESTARTSYS
>
> down_read_trylock can't be interrupted and so ERESTARTSYS would reach
> userspace, which is not permitted. Change it to EAGAIN
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>
> ---
> commit 235cf594bc65128250632a642f3e9d7e4df4975e
> tree 0557746416827daeb4d829610fec2f0c9111a675
> parent db38c559d37219c32b179ae005ca7e489336ec94
> author Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:15:47 +0200
> committer Jiri Slaby <jirislaby@gmail.com> Mon, 08 Oct 2007 14:15:47 +0200
>
>  drivers/media/video/zc0301/zc0301_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/zc0301/zc0301_core.c
> b/drivers/media/video/zc0301/zc0301_core.c index 35138c5..08a93c3 100644
> --- a/drivers/media/video/zc0301/zc0301_core.c
> +++ b/drivers/media/video/zc0301/zc0301_core.c
> @@ -655,7 +655,7 @@ static int zc0301_open(struct inode* inode, struct
> file* filp) int err = 0;
>
>  	if (!down_read_trylock(&zc0301_dev_lock))
> -		return -ERESTARTSYS;
> +		return -EAGAIN;
>
>  	cam = video_get_drvdata(video_devdata(filp));

Re: [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Mauro Carvalho Chehab]

Hi Jiri,

Em Seg, 2007-10-08 às 13:41 +0100, Jiri Slaby escreveu:
> cinergyT2, remove bad usage of ERESTARTSYS
> 
> test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
> ERESTARTSYS would reach userspace, which is not permitted. Change it to
> EAGAIN
> 

checkpatch.pl is complaining about your changeset:

do not use assignment in if condition
#82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
+     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))

do not use assignment in if condition
#86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
+     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

do not use assignment in if condition
#133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
+     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))

do not use assignment in if condition
#137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
+     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

Please fix.

Cheers,
Mauro

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Michael Krufky]

Mauro Carvalho Chehab wrote:
> Hi Jiri,
>
> Em Seg, 2007-10-08 às 13:41 +0100, Jiri Slaby escreveu:
>   
>> cinergyT2, remove bad usage of ERESTARTSYS
>>
>> test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
>> ERESTARTSYS would reach userspace, which is not permitted. Change it to
>> EAGAIN
>>
>>     
>
> checkpatch.pl is complaining about your changeset:
>
> do not use assignment in if condition
> #82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
> +     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
>
> do not use assignment in if condition
> #86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
> +     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
>
> do not use assignment in if condition
> #133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
> +     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
>
> do not use assignment in if condition
> #137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
> +     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))

Is this illegal as per kernel codingstyle?  I could understand if we may
want to avoid this sort of thing for the sake of code readability, but
this seems 100% proper to me, especially considering that we're simply
trying to catch an error return code.

One of the things that I really enjoy about the c programming language
is the fact that you can string many operations together into a single
statement through the use of logic.  I hate the thought of a patch being
nacked because of the above.  :-/

If this is indeed kernel codingstyle, then IMHO, we should let it slide
for catching error return codes.

Regards,

Mike Krufky

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Mauro Carvalho Chehab]

Em Qua, 2007-10-10 às 00:18 -0400, Michael Krufky escreveu:
> 
> Is this illegal as per kernel codingstyle?

Yes, it is. CodingStyle states:

"Don't put multiple statements on a single line unless you have
something to hide"
	and
"Don't put multiple assignments on a single line either.  Kernel coding style
is super simple.  Avoid tricky expressions."

So, Kernel's script/checkpatch.pl is right when complaining about it.

>   I could understand if we may
> want to avoid this sort of thing for the sake of code readability, but
> this seems 100% proper to me, especially considering that we're simply
> trying to catch an error return code.

> One of the things that I really enjoy about the c programming language
> is the fact that you can string many operations together into a single
> statement through the use of logic.

Yes, this is a great C feature, especially to obfuscate a source code.
On C, it is possible to write very complex code, with several
statements, on a single clause, like:

if((c=(a=x,x-=c,++a)>6?1:-1)>0)goto foo;

The above code is valid under C, and won't produce a single compiler
warning. An experienced C programmer will understand the above code,
while non-experienced ones, even with large experience on other
programming languages, may take hours to understand.

A large code, with lots of the above style will be very painful to
analyze, even for advanced programmers. So, especially on big projects,
with lots of contributors, this should really be avoided. 

> I hate the thought of a patch being nacked because of the above.  :-/
> If this is indeed kernel codingstyle, then IMHO, we should let it slide
> for catching error return codes.

It is just a matter of a simple CodingStyle fix.

The proper fix is just to replace the offended code by this:

err=foo();
if (error)
	goto error;

-- 
Cheers,
Mauro

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Alan Cox]

On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
> Em Qua, 2007-10-10 às 00:18 -0400, Michael Krufky escreveu:
> > 
> > Is this illegal as per kernel codingstyle?
> 
> Yes, it is. CodingStyle states:

<rant>
No.. "Illegal" means prohibited by law. Its merely wrong 8)
</rant>

> The proper fix is just to replace the offended code by this:
> 
> err=foo();
> if (error)
> 	goto error;

Lots of code uses

	if ((err = foo()) < 0)

so I would'y worry too much. The split one however clearer and also
safer.

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Mauro Carvalho Chehab]

Em Qua, 2007-10-10 às 11:59 -0400, Alan Cox escreveu:
> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
> > Em Qua, 2007-10-10 às 00:18 -0400, Michael Krufky escreveu:
> > > 
> > > Is this illegal as per kernel codingstyle?
> > 
> > Yes, it is. CodingStyle states:
> 
> <rant>
> No.. "Illegal" means prohibited by law. Its merely wrong 8)
> </rant>

LOL

> > The proper fix is just to replace the offended code by this:
> > 
> > err=foo();
> > if (error)
> > 	goto error;
> 
> Lots of code uses
> 
> 	if ((err = foo()) < 0)
> 
> so I would'y worry too much. The split one however clearer and also
> safer.

Yes, this is not a severe CodingStyle violation. Still, the above code
is better than the used one.

Since, on your example, it is clear that the programmer wanted to test
if the value is less than zero. 

The code:

	if ( (err=foo()) )

should also indicate an operator mistake of using =, instead of ==.

Probably, source code analyzers like Coverity will complain about the
above.

If not violating CodingStyle, I would rather prefer to code this as:
	if ( !(err=foo() ) 
or, even better, using:
	if ( (err=foo()) != 0)

clearly indicating that it is tested if the value is not zero.

Even being a quite simple issue, I would prefer if Jiri can fix it.

-- 
Cheers,
Mauro

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Manu Abraham]

Mauro Carvalho Chehab wrote:
> Em Qua, 2007-10-10 Ã s 11:59 -0400, Alan Cox escreveu:
>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
>>> Em Qua, 2007-10-10 Ã s 00:18 -0400, Michael Krufky escreveu:
>>>> Is this illegal as per kernel codingstyle?
>>> Yes, it is. CodingStyle states:
>> <rant>
>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
>> </rant>
> 
> LOL
> 
>>> The proper fix is just to replace the offended code by this:
>>>
>>> err=foo();
>>> if (error)
>>> 	goto error;
>> Lots of code uses
>>
>> 	if ((err = foo()) < 0)
>>
>> so I would'y worry too much. The split one however clearer and also
>> safer.
> 
> Yes, this is not a severe CodingStyle violation. Still, the above code
> is better than the used one.
> 
> Since, on your example, it is clear that the programmer wanted to test
> if the value is less than zero. 
> 
> The code:
> 
> 	if ( (err=foo()) )
> 
> should also indicate an operator mistake of using =, instead of ==.
> 
> Probably, source code analyzers like Coverity will complain about the
> above.
> 
> If not violating CodingStyle, I would rather prefer to code this as:
> 	if ( !(err=foo() ) 
> or, even better, using:
> 	if ( (err=foo()) != 0)
> 
> clearly indicating that it is tested if the value is not zero.
> 
> Even being a quite simple issue, I would prefer if Jiri can fix it.
> 


When you have only some few lines of code you can write

 err = foo()
 if (err) {
  do whatever
 } 

doesn't matter ..

But when you have hell a lot of code, checking error's what you 
mention is insane.

ie,

if ((err = foo()) expr ) is better.

http://lkml.org/lkml/2007/2/4/56

Manu

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Marcel Siegert]

Manu Abraham schrieb:
> Mauro Carvalho Chehab wrote:
>> Em Qua, 2007-10-10 Ã s 11:59 -0400, Alan Cox escreveu:
>>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
>>>> Em Qua, 2007-10-10 Ã s 00:18 -0400, Michael Krufky escreveu:
>>>>> Is this illegal as per kernel codingstyle?
>>>> Yes, it is. CodingStyle states:
>>> <rant>
>>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
>>> </rant>
>> LOL
>>
>>>> The proper fix is just to replace the offended code by this:
>>>>
>>>> err=foo();
>>>> if (error)
>>>> 	goto error;
>>> Lots of code uses
>>>
>>> 	if ((err = foo()) < 0)
>>>
>>> so I would'y worry too much. The split one however clearer and also
>>> safer.
>> Yes, this is not a severe CodingStyle violation. Still, the above code
>> is better than the used one.
>>
>> Since, on your example, it is clear that the programmer wanted to test
>> if the value is less than zero. 
>>
>> The code:
>>
>> 	if ( (err=foo()) )
>>
>> should also indicate an operator mistake of using =, instead of ==.
>>
>> Probably, source code analyzers like Coverity will complain about the
>> above.
>>
>> If not violating CodingStyle, I would rather prefer to code this as:
>> 	if ( !(err=foo() ) 
>> or, even better, using:
>> 	if ( (err=foo()) != 0)
>>
>> clearly indicating that it is tested if the value is not zero.
>>
>> Even being a quite simple issue, I would prefer if Jiri can fix it.
>>
> 
> 
> When you have only some few lines of code you can write
> 
>  err = foo()
>  if (err) {
>   do whatever
>  } 
> 
> doesn't matter ..
> 
> But when you have hell a lot of code, checking error's what you 
> mention is insane.
> 
> ie,
> 
> if ((err = foo()) expr ) is better.
> 
> http://lkml.org/lkml/2007/2/4/56
> 
> Manu
> 
hi manu,

it's not worth discussing this in a way like
"i know something from the past and that was a different solution".

if you look to other parts in that thread like

http://lkml.org/lkml/2007/2/3/150

you will see that they came also to a kind of different solution,
NOT to use the 1-liner for assignment statements.

it's more like a religious/personal discussion how to 
struct/indent/format code.
and, to state my position for clear,

if kernel coding style document isnt updated to allow the constructions
of code that caused this discussion, we dont have to discuss but follow 
the rules.

anything else on this topic (coding style and it's sense) is to be 
discussed on kernel ml.

my 2ct
marcel



> _______________________________________________
> v4l-dvb-maintainer mailing list
> v4l-dvb-maintainer@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Manu Abraham]

Marcel Siegert wrote:
> Manu Abraham schrieb:
>> Mauro Carvalho Chehab wrote:
>>> Em Qua, 2007-10-10 Ã s 11:59 -0400, Alan Cox escreveu:
>>>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
>>>>> Em Qua, 2007-10-10 Ã s 00:18 -0400, Michael Krufky escreveu:
>>>>>> Is this illegal as per kernel codingstyle?
>>>>> Yes, it is. CodingStyle states:
>>>> <rant>
>>>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
>>>> </rant>
>>> LOL
>>>
>>>>> The proper fix is just to replace the offended code by this:
>>>>>
>>>>> err=foo();
>>>>> if (error)
>>>>>     goto error;
>>>> Lots of code uses
>>>>
>>>>     if ((err = foo()) < 0)
>>>>
>>>> so I would'y worry too much. The split one however clearer and also
>>>> safer.
>>> Yes, this is not a severe CodingStyle violation. Still, the above code
>>> is better than the used one.
>>>
>>> Since, on your example, it is clear that the programmer wanted to test
>>> if the value is less than zero.
>>> The code:
>>>
>>>     if ( (err=foo()) )
>>>
>>> should also indicate an operator mistake of using =, instead of ==.
>>>
>>> Probably, source code analyzers like Coverity will complain about the
>>> above.
>>>
>>> If not violating CodingStyle, I would rather prefer to code this as:
>>>     if ( !(err=foo() ) or, even better, using:
>>>     if ( (err=foo()) != 0)
>>>
>>> clearly indicating that it is tested if the value is not zero.
>>>
>>> Even being a quite simple issue, I would prefer if Jiri can fix it.
>>>
>>
>>
>> When you have only some few lines of code you can write
>>
>>  err = foo()
>>  if (err) {
>>   do whatever
>>  }
>> doesn't matter ..
>>
>> But when you have hell a lot of code, checking error's what you
>> mention is insane.
>>
>> ie,
>>
>> if ((err = foo()) expr ) is better.
>>
>> http://lkml.org/lkml/2007/2/4/56
>>
>> Manu
>>
> hi manu,
> 
> it's not worth discussing this in a way like
> "i know something from the past and that was a different solution".
> 

didn't mean to look at it that way, because i had addressed my concerns 
at that time as well.

> if you look to other parts in that thread like
> 
> http://lkml.org/lkml/2007/2/3/150
> 
> you will see that they came also to a kind of different solution,
> NOT to use the 1-liner for assignment statements.
> 
> it's more like a religious/personal discussion how to
> struct/indent/format code.
> and, to state my position for clear,


It is. Sometimes i find such things in CodingStyle to be too silly.

> 
> if kernel coding style document isnt updated to allow the constructions
> of code that caused this discussion, we dont have to discuss but follow
> the rules.
> 
> anything else on this topic (coding style and it's sense) is to be
> discussed on kernel ml.
> 

Marcel, It is on LKML.

Manu

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Marcel Siegert]

Manu Abraham schrieb:
> Marcel Siegert wrote:
>> Manu Abraham schrieb:
>>> Mauro Carvalho Chehab wrote:
>>>> Em Qua, 2007-10-10 Ã s 11:59 -0400, Alan Cox escreveu:
>>>>> On Wed, Oct 10, 2007 at 12:35:41PM -0300, Mauro Carvalho Chehab wrote:
>>>>>> Em Qua, 2007-10-10 Ã s 00:18 -0400, Michael Krufky escreveu:
>>>>>>> Is this illegal as per kernel codingstyle?
>>>>>> Yes, it is. CodingStyle states:
>>>>> <rant>
>>>>> No.. "Illegal" means prohibited by law. Its merely wrong 8)
>>>>> </rant>
>>>> LOL
>>>>
>>>>>> The proper fix is just to replace the offended code by this:
>>>>>>
>>>>>> err=foo();
>>>>>> if (error)
>>>>>>     goto error;
>>>>> Lots of code uses
>>>>>
>>>>>     if ((err = foo()) < 0)
>>>>>
>>>>> so I would'y worry too much. The split one however clearer and also
>>>>> safer.
>>>> Yes, this is not a severe CodingStyle violation. Still, the above code
>>>> is better than the used one.
>>>>
>>>> Since, on your example, it is clear that the programmer wanted to test
>>>> if the value is less than zero.
>>>> The code:
>>>>
>>>>     if ( (err=foo()) )
>>>>
>>>> should also indicate an operator mistake of using =, instead of ==.
>>>>
>>>> Probably, source code analyzers like Coverity will complain about the
>>>> above.
>>>>
>>>> If not violating CodingStyle, I would rather prefer to code this as:
>>>>     if ( !(err=foo() ) or, even better, using:
>>>>     if ( (err=foo()) != 0)
>>>>
>>>> clearly indicating that it is tested if the value is not zero.
>>>>
>>>> Even being a quite simple issue, I would prefer if Jiri can fix it.
>>>>
>>>
>>> When you have only some few lines of code you can write
>>>
>>>  err = foo()
>>>  if (err) {
>>>   do whatever
>>>  }
>>> doesn't matter ..
>>>
>>> But when you have hell a lot of code, checking error's what you
>>> mention is insane.
>>>
>>> ie,
>>>
>>> if ((err = foo()) expr ) is better.
>>>
>>> http://lkml.org/lkml/2007/2/4/56
>>>
>>> Manu
>>>
>> hi manu,
>>
>> it's not worth discussing this in a way like
>> "i know something from the past and that was a different solution".
>>
> 
> didn't mean to look at it that way, because i had addressed my concerns 
> at that time as well.
> 
>> if you look to other parts in that thread like
>>
>> http://lkml.org/lkml/2007/2/3/150
>>
>> you will see that they came also to a kind of different solution,
>> NOT to use the 1-liner for assignment statements.
>>
>> it's more like a religious/personal discussion how to
>> struct/indent/format code.
>> and, to state my position for clear,
> 
> 
> It is. Sometimes i find such things in CodingStyle to be too silly.
> 
>> if kernel coding style document isnt updated to allow the constructions
>> of code that caused this discussion, we dont have to discuss but follow
>> the rules.
>>
>> anything else on this topic (coding style and it's sense) is to be
>> discussed on kernel ml.
>>
> 
> Marcel, It is on LKML.

i do know manu, but as far as i can see from my fresh 2.6.23,
its not solved or changed in vanilla kernel CodingStyle doc.

so we have to follow actual guidelines _or_ wait until
CodingStyle is accordingly updated.

not more, not less.


regards
marcel

Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Randy Dunlap]

On Wed, 10 Oct 2007 00:18:28 -0400 Michael Krufky wrote:

> Mauro Carvalho Chehab wrote:
> > Hi Jiri,
> >
> > Em Seg, 2007-10-08 às 13:41 +0100, Jiri Slaby escreveu:
> >   
> >> cinergyT2, remove bad usage of ERESTARTSYS
> >>
> >> test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
> >> ERESTARTSYS would reach userspace, which is not permitted. Change it to
> >> EAGAIN
> >>
> >>     
> >
> > checkpatch.pl is complaining about your changeset:
> >
> > do not use assignment in if condition
> > #82: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:492:
> > +     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
> >
> > do not use assignment in if condition
> > #86: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:495:
> > +     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
> >
> > do not use assignment in if condition
> > #133: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1036:
> > +     if ((err = mutex_lock_interruptible(&cinergyt2->wq_sem)))
> >
> > do not use assignment in if condition
> > #137: FILE: drivers/media/dvb/cinergyT2/cinergyT2.c:1039:
> > +     if ((err = mutex_lock_interruptible(&cinergyt2->sem)))
> 
> Is this illegal as per kernel codingstyle?  I could understand if we may
> want to avoid this sort of thing for the sake of code readability, but
> this seems 100% proper to me, especially considering that we're simply
> trying to catch an error return code.

Anyway, it's not strictly from CodingStyle.  The closest that
CodingStyle has to say about it IMO is this:

"Don't put multiple assignments on a single line either.  Kernel coding style
is super simple.  Avoid tricky expressions."

Also, Andrew Morton and Christoph Hellwig push for splitting up
the if-assignment lines, so it's a trend over the past few
(probably) years.  It's not real new.


> One of the things that I really enjoy about the c programming language
> is the fact that you can string many operations together into a single
> statement through the use of logic.  I hate the thought of a patch being
> nacked because of the above.  :-/

so you like the challenge of reading obfuscated code?

At any rate, it's rare that a patch is nacked only due to coding style,
unless it's blatant and occurs many times (like throughout the entire
source file).


> If this is indeed kernel codingstyle, then IMHO, we should let it slide
> for catching error return codes.

Nope.

---
~Randy

[PATCH 1/1 try 2] V4L: cinergyT2, remove bad usage of ERESTARTSYS

[Jiri Slaby]

cinergyT2, remove bad usage of ERESTARTSYS

test of cinergyt2->disconnect_pending doesn't ensure pending signal and so
ERESTARTSYS would reach userspace, which is not permitted. Change it to
EAGAIN

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 4b741d759e5b898bf1bf19631d5e5b14a221ce52
tree af90dcd22292fc4fee4b3337118e6cb527796499
parent f87566db6dd9613dde8de59380cd2f423166511e
author Jiri Slaby <jirislaby@gmail.com> Sun, 14 Oct 2007 16:25:55 +0200
committer Jiri Slaby <jirislaby@gmail.com> Sun, 14 Oct 2007 16:25:55 +0200

 drivers/media/dvb/cinergyT2/cinergyT2.c |   42 +++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/media/dvb/cinergyT2/cinergyT2.c b/drivers/media/dvb/cinergyT2/cinergyT2.c
index 154a7ce..7199e4c 100644
--- a/drivers/media/dvb/cinergyT2/cinergyT2.c
+++ b/drivers/media/dvb/cinergyT2/cinergyT2.c
@@ -345,7 +345,9 @@ static int cinergyt2_start_feed(struct dvb_demux_feed *dvbdmxfeed)
 	struct dvb_demux *demux = dvbdmxfeed->demux;
 	struct cinergyt2 *cinergyt2 = demux->priv;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	if (cinergyt2->streaming == 0)
@@ -361,7 +363,9 @@ static int cinergyt2_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
 	struct dvb_demux *demux = dvbdmxfeed->demux;
 	struct cinergyt2 *cinergyt2 = demux->priv;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	if (--cinergyt2->streaming == 0)
@@ -481,12 +485,16 @@ static int cinergyt2_open (struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct cinergyt2 *cinergyt2 = dvbdev->priv;
-	int err = -ERESTARTSYS;
+	int err = -EAGAIN;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		goto out;
+	err = mutex_lock_interruptible(&cinergyt2->wq_sem);
+	if (err)
 		goto out;
 
-	if (mutex_lock_interruptible(&cinergyt2->sem))
+	err = mutex_lock_interruptible(&cinergyt2->sem);
+	if (err)
 		goto out_unlock1;
 
 	if ((err = dvb_generic_open(inode, file)))
@@ -550,7 +558,9 @@ static unsigned int cinergyt2_poll (struct file *file, struct poll_table_struct
 	struct cinergyt2 *cinergyt2 = dvbdev->priv;
 	unsigned int mask = 0;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->sem))
 		return -ERESTARTSYS;
 
 	poll_wait(file, &cinergyt2->poll_wq, wait);
@@ -625,7 +635,9 @@ static int cinergyt2_ioctl (struct inode *inode, struct file *file,
 		if (copy_from_user(&p, (void  __user*) arg, sizeof(p)))
 			return -EFAULT;
 
-		if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->sem))
+		if (cinergyt2->disconnect_pending)
+			return -EAGAIN;
+		if (mutex_lock_interruptible(&cinergyt2->sem))
 			return -ERESTARTSYS;
 
 		param->cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS;
@@ -996,7 +1008,9 @@ static int cinergyt2_suspend (struct usb_interface *intf, pm_message_t state)
 {
 	struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		return -EAGAIN;
+	if (mutex_lock_interruptible(&cinergyt2->wq_sem))
 		return -ERESTARTSYS;
 
 	cinergyt2_suspend_rc(cinergyt2);
@@ -1017,16 +1031,18 @@ static int cinergyt2_resume (struct usb_interface *intf)
 {
 	struct cinergyt2 *cinergyt2 = usb_get_intfdata (intf);
 	struct dvbt_set_parameters_msg *param = &cinergyt2->param;
-	int err = -ERESTARTSYS;
+	int err = -EAGAIN;
 
-	if (cinergyt2->disconnect_pending || mutex_lock_interruptible(&cinergyt2->wq_sem))
+	if (cinergyt2->disconnect_pending)
+		goto out;
+	err = mutex_lock_interruptible(&cinergyt2->wq_sem);
+	if (err)
 		goto out;
 
-	if (mutex_lock_interruptible(&cinergyt2->sem))
+	err = mutex_lock_interruptible(&cinergyt2->sem);
+	if (err)
 		goto out_unlock1;
 
-	err = 0;
-
 	if (!cinergyt2->sleeping) {
 		cinergyt2_sleep(cinergyt2, 0);
 		cinergyt2_command(cinergyt2, (char *) param, sizeof(*param), NULL, 0);