Home » Mailing list archives » linux-kernel » 2007 » October » 9

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

Previous thread: [PATCH]fix VM_CAN_NONLINEAR check in sys_remap_file_pages by Yan Zheng on Monday, October 8, 2007 - 4:45 am. (11 messages)

Next thread: [PATCH 1/1] V4L: rocket, switch sleep_on to completion by Jiri Slaby on Monday, October 8, 2007 - 5:35 am. (2 messages)
[view original message]
From: Jiri Slaby
Subject: [PATCH 1/3] V4L: w9968cf, remove bad usage of ERESTARTSYS
Date: Monday, October 8, 2007 - 5:33 am

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));
 
-


[ message continues ]
[view original message]
From: Jiri Slaby
Subject: [PATCH 2/3] V4L: zc0301, remove bad usage of ERESTARTSYS
Date: Monday, October 8, 2007 - 5:34 am

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));
 
-


[ message continues ]
[view original message]
From: Luca Risolia
Subject: Re: [PATCH 2/3] V4L: zc0301, remove bad usage of ERESTARTSYS
Date: Tuesday, October 9, 2007 - 5:18 pm

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




-


[ message continues ]
[view original message]
From: Luca Risolia
Subject: Re: [PATCH 1/3] V4L: w9968cf, remove bad usage of ERESTARTSYS
Date: Tuesday, October 9, 2007 - 5:18 pm

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



-


[ message continues ]
[view original message]
From: Jiri Slaby
Subject: [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Monday, October 8, 2007 - 5:34 am

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 ...
[ message continues ]
[view original message]
From: Mauro Carvalho Chehab
Subject: Re: [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Tuesday, October 9, 2007 - 6:21 pm

Hi Jiri,


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

-


[ message continues ]
[view original message]
From: Michael Krufky
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Tuesday, October 9, 2007 - 9:18 pm

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
-


[ message continues ]
[view original message]
From: Mauro Carvalho Chehab
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 8:35 am

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."


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,

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

-


[ message continues ]
[view original message]
From: Alan Cox
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 8:59 am

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

Lots of code uses

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

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

-


[ message continues ]
[view original message]
From: Mauro Carvalho Chehab
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 9:17 am

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

-


[ message continues ]
[view original message]
From: Manu Abraham
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 9:40 am

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
-


[ message continues ]
[view original message]
From: Marcel Siegert
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 9:53 am

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

-


[ message continues ]
[view original message]
From: Manu Abraham
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 10:05 am

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



Marcel, It is on LKML.

Manu

-


[ message continues ]
[view original message]
From: Marcel Siegert
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Wednesday, October 10, 2007 - 11:04 am

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

-


[ message continues ]
[view original message]
From: Randy Dunlap
Subject: Re: [v4l-dvb-maintainer] [PATCH 3/3] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Thursday, October 11, 2007 - 9:29 pm

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

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

Nope.

---
~Randy
-


[ message continues ]
[view original message]
From: Jiri Slaby
Subject: [PATCH 1/1 try 2] V4L: cinergyT2, remove bad usage of ERESTARTSYS
Date: Sunday, October 14, 2007 - 7:28 am

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 ...
[ message continues ]
Previous thread: [PATCH]fix VM_CAN_NONLINEAR check in sys_remap_file_pages by Yan Zheng on Monday, October 8, 2007 - 4:45 am. (11 messages)

Next thread: [PATCH 1/1] V4L: rocket, switch sleep_on to completion by Jiri Slaby on Monday, October 8, 2007 - 5:35 am. (2 messages)