On Mon, Mar 12, 2007 at 10:57:53AM +0300, Dmitriy Monakhov wrote:
> I realy don't want to be annoying by sending this patcheset over and over
> again. If anyone think this patch is realy cappy, please comment what
> exectly is bad. Thank you.
Doesn't seem like a bad idea.
>
> Changes:
> - patch was split in two patches.
> - comments added. I think now it is clearly describe things.
> - make generic_segment_checks() inline
> - patch prepared against 2.6.20-mm3
>
> How is tested:
> - LTP test, and other readv/writev op tests.
>
> Signed-off-by: Monakhov Dmitriy <[EMAIL PROTECTED]>
> ---
> mm/filemap.c | 76 +++++++++++++++++++++++++++++----------------------------
> 1 files changed, 39 insertions(+), 37 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 8e1849a..0aadf5f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1159,6 +1159,39 @@ success:
> return size;
> }
>
> +/*
> + * Performs necessary checks before doing a write
> + *
> + * Adjust number of segments and amount of bytes to write.
> + * Returns appropriate error code that caller should return or
> + * zero in case that write should be allowed.
> + */
> +inline int generic_segment_checks(const struct iovec *iov,
> + unsigned long *nr_segs, size_t *count,
> + unsigned long access_flags)
Make it static and not inline, and the compiler will work it out.
This function name doesn't really imply that it returns you the
nr_segs and count, but that's not a big deal I guess.
You also don't say that nr_segs should be initialised to the amount
you which to write, while count must be initialised to zero.
> +{
> + unsigned long seg;
> + for (seg = 0; seg < *nr_segs; seg++) {
> + const struct iovec *iv = &iov[seg];
> +
> + /*
> + * If any segment has a negative length, or the cumulative
> + * length ever wraps negative then return -EINVAL.
> + */
> + *count += iv->iov_len;
> + if (unlikely((ssize_t)(*count|iv->iov_len) < 0))
> + return -EINVAL;
> + if (access_ok(access_flags, iv->iov_base, iv->iov_len))
> + continue;
Why now insert the above test, and put the below statements inside the
branch? OTOH, that makes it less obviously c&p from the others. Maybe
a subsequent patch.
> + if (seg == 0)
> + return -EFAULT;
> + *nr_segs = seg;
> + *count -= iv->iov_len; /* This segment is no good */
> + break;
> + }
You could assign to *count here, once, and remove the requirement
that the caller initialised it to zero?
> + return 0;
> +}
> +
> /**
> * generic_file_aio_read - generic filesystem read routine
> * @iocb: kernel I/O control block
> @@ -1180,24 +1213,9 @@ generic_file_aio_read(struct kiocb *iocb, const struct
> iovec *iov,
> loff_t *ppos = &iocb->ki_pos;
>
> count = 0;
> - for (seg = 0; seg < nr_segs; seg++) {
> - const struct iovec *iv = &iov[seg];
> -
> - /*
> - * If any segment has a negative length, or the cumulative
> - * length ever wraps negative then return -EINVAL.
> - */
> - count += iv->iov_len;
> - if (unlikely((ssize_t)(count|iv->iov_len) < 0))
> - return -EINVAL;
> - if (access_ok(VERIFY_WRITE, iv->iov_base, iv->iov_len))
> - continue;
> - if (seg == 0)
> - return -EFAULT;
> - nr_segs = seg;
> - count -= iv->iov_len; /* This segment is no good */
> - break;
> - }
> + retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> + if (retval)
> + return retval;
>
> /* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
> if (filp->f_flags & O_DIRECT) {
> @@ -2094,30 +2112,14 @@ __generic_file_aio_write_nolock(struct kiocb *iocb,
> const struct iovec *iov,
> size_t ocount; /* original count */
> size_t count; /* after file limit checks */
> struct inode *inode = mapping->host;
> - unsigned long seg;
> loff_t pos;
> ssize_t written;
> ssize_t err;
>
> ocount = 0;
> - for (seg = 0; seg < nr_segs; seg++) {
> - const struct iovec *iv = &iov[seg];
> -
> - /*
> - * If any segment has a negative length, or the cumulative
> - * length ever wraps negative then return -EINVAL.
> - */
> - ocount += iv->iov_len;
> - if (unlikely((ssize_t)(ocount|iv->iov_len) < 0))
> - return -EINVAL;
> - if (access_ok(VERIFY_READ, iv->iov_base, iv->iov_len))
> - continue;
> - if (seg == 0)
> - return -EFAULT;
> - nr_segs = seg;
> - ocount -= iv->iov_len; /* This segment is no good */
> - break;
> - }
> + err = generic_segment_checks(iov, &nr_segs, &ocount, VERIFY_READ);
> + if (err)
> + return err;
>
> count = ocount;
> pos = *ppos;
> --
> 1.5.0.1
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
