2006-05-10 02:58:52

by Daniel Walker

Subject: [PATCH -mm] idetape gcc 4.1 warning fix

Fixes the following warning,

drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_from_user’:
drivers/ide/ide-tape.c:2662: warning: ignoring return value of ‘copy_from_user’, declared with attribute warn_unused_result
drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_to_user’:
drivers/ide/ide-tape.c:2689: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result

Signed-Off-By: Daniel Walker <[email protected]>

Index: linux-2.6.16/drivers/ide/ide-tape.c
===================================================================
--- linux-2.6.16.orig/drivers/ide/ide-tape.c
+++ linux-2.6.16/drivers/ide/ide-tape.c
@@ -2659,7 +2659,7 @@ static void idetape_copy_stage_from_user
}
#endif /* IDETAPE_DEBUG_BUGS */
count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
- copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
+ WARN_ON(copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count));
n -= count;
atomic_add(count, &bh->b_count);
buf += count;
@@ -2686,7 +2686,7 @@ static void idetape_copy_stage_to_user (
}
#endif /* IDETAPE_DEBUG_BUGS */
count = min(tape->b_count, n);
- copy_to_user(buf, tape->b_data, count);
+ WARN_ON(copy_to_user(buf, tape->b_data, count));
n -= count;
tape->b_data += count;
tape->b_count -= count;

2006-05-10 10:30:21

by Alan

Subject: Re: [PATCH -mm] idetape gcc 4.1 warning fix

On Maw, 2006-05-09 at 19:55 -0700, Daniel Walker wrote:
> Fixes the following warning,
>
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_from_user’:
> drivers/ide/ide-tape.c:2662: warning: ignoring return value of ‘copy_from_user’, declared with attribute warn_unused_result
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_to_user’:

> count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> + WARN_ON(copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count));

So you want to let users spew all over the kernel log when a copy from
user fails. Either fix it properly or leave it alone. In this case its
actually quite hard to fix properly which is why it hasn't been done.

(POSIX doesn't require invalid addresses reliably report -EFAULT)

2006-05-10 11:12:13

by Jakub Jelinek

Subject: Re: [PATCH -mm] idetape gcc 4.1 warning fix

On Tue, May 09, 2006 at 07:55:58PM -0700, Daniel Walker wrote:
> Fixes the following warning,
>
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_from_user’:
> drivers/ide/ide-tape.c:2662: warning: ignoring return value of ‘copy_from_user’, declared with attribute warn_unused_result
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_to_user’:
> drivers/ide/ide-tape.c:2689: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result
>
> Signed-Off-By: Daniel Walker <[email protected]>
>
> Index: linux-2.6.16/drivers/ide/ide-tape.c
> ===================================================================
> --- linux-2.6.16.orig/drivers/ide/ide-tape.c
> +++ linux-2.6.16/drivers/ide/ide-tape.c
> @@ -2659,7 +2659,7 @@ static void idetape_copy_stage_from_user
> }
> #endif /* IDETAPE_DEBUG_BUGS */
> count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> + WARN_ON(copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count));

WARN_ON is certainly not a good way to hide this warning.
Having a user-triggerable WARN_ON is a bad idea.
Instead you should add some error handling.

Jakub

2006-05-10 14:07:17

by Daniel Walker

Subject: Re: [PATCH -mm] idetape gcc 4.1 warning fix

On Wed, 2006-05-10 at 07:12 -0400, Jakub Jelinek wrote:

> WARN_ON is certainly not a good way to hide this warning.
> Having a user-triggerable WARN_ON is a bad idea.
> Instead you should add some error handling.

This was one of 3 or so functions that had no return facility . I had a
feeling people wouldn't agree with the WARN_ON() , so I'm hoping someone
has a better solution . Could just be a printk , but I'm not going to
revamp the drivers error handling just to cure the warning .

Daniel