Re: [PATCH, RFC 21/30] isdn: divert, hysdn: fix interruptible_sleep_onrace

From: Sergei Shtylyov
Date: Thu Jan 02 2014 - 17:01:03 EST


On 01/02/2014 07:48 PM, Arnd Bergmann wrote:

diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
index fb4f1ba..1c5dc34 100644
--- a/drivers/isdn/divert/divert_procfs.c
+++ b/drivers/isdn/divert/divert_procfs.c
@@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
struct divert_info *inf;
int len;

- if (!*((struct divert_info **) file->private_data)) {
+ if (!(inf = *((struct divert_info **) file->private_data))) { shouldn't approve assignment inside *if*. Though you're
moving it from the existing code, it wouldn't hurt to fix it.

I tried to touch as little as possible, and while I wouldn't use that
style myself, it is applied consistently in this driver, including the
wait_event line I'm adding, where I feel it actually makes sense.

I wasn't feeling sure about that one. It seems to me now that it doesn't make much sense -- we could do the assignment beforehand.

if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- interruptible_sleep_on(&(rd_queue));
+ wait_event_interruptible(rd_queue, (inf =
+ *((struct divert_info **) file->private_data)));

Parens around assignment are hardly useful.

We get a gcc warning without them:

drivers/isdn/divert/divert_procfs.c:95:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
*((struct divert_info **) file->private_data));


I can still change the first one (in both files) if you think it's important,

I always prefer patches, though some people do argue when they are just moving the badly styled code around.

but I'd rather not spend too much energy at coding style changes.

Let's wait for the maintainer's opinion on this.


WBR, Sergei

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at