| Message ID | 1238438693-29540-7-git-send-email-andy.grover@oracle.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
> - if (0 <= ret && (unsigned) ret < nr_pages) { > + if (ret > 0 && (unsigned) ret < nr_pages) { This is not an equivalent transformation -- the original code is true if ret == 0, while the new code is false. Also it seems you don't need the unsigned cast here, since the clause before just checked that ret is positive? - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 30, 2009 at 9:27 PM, Roland Dreier <rdreier@cisco.com> wrote: > > - if (0 <= ret && (unsigned) ret < nr_pages) { > > + if (ret > 0 && (unsigned) ret < nr_pages) { > > This is not an equivalent transformation -- the original code is true if > ret == 0, while the new code is false. Ah! Good point. > Also it seems you don't need the unsigned cast here, since the clause > before just checked that ret is positive? True, but I'd bet the compiler will warn if we remove it. I'll try it tomorrow and see. Thanks! -- Regards -- Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andrew Grover <andy.grover@gmail.com> Date: Mon, 30 Mar 2009 23:56:14 -0700 > On Mon, Mar 30, 2009 at 9:27 PM, Roland Dreier <rdreier@cisco.com> wrote: > > > - if (0 <= ret && (unsigned) ret < nr_pages) { > > > + if (ret > 0 && (unsigned) ret < nr_pages) { > > > > This is not an equivalent transformation -- the original code is true if > > ret == 0, while the new code is false. > > Ah! Good point. > > > Also it seems you don't need the unsigned cast here, since the clause > > before just checked that ret is positive? > > True, but I'd bet the compiler will warn if we remove it. I'll try it > tomorrow and see. Andy, also please resubmit only the real honest-to-goodness bug fixes in this patch series. I don't want to see cleanups, or optimizations like the transformation over to using get_user_pages_fast(). You could have sent that kind of stuff to me weeks ago. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/rds/rdma.c b/net/rds/rdma.c index eaeeb91..584eac3 100644 --- a/net/rds/rdma.c +++ b/net/rds/rdma.c @@ -155,7 +155,7 @@ static int rds_pin_pages(unsigned long user_addr, unsigned int nr_pages, nr_pages, write, 0, pages, NULL); up_read(¤t->mm->mmap_sem); - if (0 <= ret && (unsigned) ret < nr_pages) { + if (ret > 0 && (unsigned) ret < nr_pages) { while (ret--) put_page(pages[ret]); ret = -EFAULT;
Putting the constant first is a supposed "best practice" that actually makes the code harder to read. Signed-off-by: Andy Grover <andy.grover@oracle.com> --- net/rds/rdma.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)