On 21.04.2006 08:14, Andrew Morton wrote: > It's simpler to code it this way: [snip] > But the code still looks wrong. If isdn_writebuf_stub() does a short write, we'll > just retry the entire write. And if that returns the same short write, we'll > retry the write again, ad infinitum. You're right of course. But as i4l has never been able to handle that case correctly, i4l drivers just don't do short writes, period. They either return a negative error code, zero to indicate "not ready", or the requested length. Every i4l driver author learns that very quickly. (At least I did. ;-) > One would expect that if a short write happened, we either bale out with an > error or we advance partway through the buffer and write some more. isdn_write() is the write method of i4l's character device. If a short write would indeed occur, just passing it on to the caller should be ok, too. So I'd propose the following, even simpler version: From: Jesper Juhl <[email protected]> isdn_writebuf_stub() forgets to detect memory allocation and uaccess errors. And when that's fixed, if a error happens the caller will just keep on looping. So change the caller to detect the error, and to return it. Signed-off-by: Jesper Juhl <[email protected]> Cc: Karsten Keil <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Tilman Schmidt <[email protected]> --- drivers/isdn/i4l/isdn_common.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff -puN linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c --- linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c 2006-03-20 06:53:29.000000000 +0100 +++ linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c 2006-04-21 15:09:41.000000000 +0200 @@ -1177,9 +1177,8 @@ isdn_write(struct file *file, const char goto out; } chidx = isdn_minor2chan(minor); - while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count) + while ((retval = isdn_writebuf_stub(drvidx, chidx, buf, count)) == 0) interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]); - retval = count; goto out; } if (minor <= ISDN_MINOR_CTRLMAX) { @@ -1951,9 +1950,10 @@ isdn_writebuf_stub(int drvidx, int chan, struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC); if (!skb) - return 0; + return -ENOMEM; skb_reserve(skb, hl); - copy_from_user(skb_put(skb, len), buf, len); + if (!copy_from_user(skb_put(skb, len), buf, len)) + return -EFAULT; ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb); if (ret <= 0) dev_kfree_skb(skb); -- Tilman Schmidt E-Mail: [email protected] Bonn, Germany Reality is that which, when you stop believing in it, does not go away. (Philip K. Dick)
Attachment:
signature.asc
Description: OpenPGP digital signature
- References:
- Prev by Date: Re: [PATCH 3/7] FS-Cache: Avoid ENFILE checking for kernel-specific open files
- Next by Date: Re: Which process is associated with process ID 0 (swapper)
- Previous by thread: Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub
- Next by thread: unix socket connection tracking
- Index(es):
 |