[PATCH] buffer::ptr::cmp only compares up to the smallest length

[PATCH] buffer::ptr::cmp only compares up to the smallest length

[Loic Dachary]

When running

  bufferptr a("A", 1);
  bufferptr ab("AB", 2);
  a.cmp(ab);

it returned zero because cmp only compared up to the length of the
smallest buffer. The tests comparing the length of the buffers are
moved before the memcmp comparing the actual content of the buffers.

http://tracker.ceph.com/issues/4170 refs #4170

Signed-off-by: Loic Dachary <loic@dachary.org>
---
 src/common/buffer.cc   |    8 +-------
 src/test/bufferlist.cc |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index e10d6c9..5a88849 100644
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -368,17 +368,11 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
 
   int buffer::ptr::cmp(const ptr& o)
   {
-    int l = _len < o._len ? _len : o._len;
-    if (l) {
-      int r = memcmp(c_str(), o.c_str(), l);
-      if (!r)
-	return r;
-    }
     if (_len < o._len)
       return -1;
     if (_len > o._len)
       return 1;
-    return 0;
+    return memcmp(c_str(), o.c_str(), _len);
   }
 
   bool buffer::ptr::is_zero() const
diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 71c2e79..aac41c6 100644
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -9,6 +9,20 @@
 
 #define MAX_TEST 1000000
 
+TEST(BufferPtr, cmp) {
+  bufferptr empty;
+  bufferptr a("A", 1);
+  bufferptr ab("AB", 2);
+  bufferptr af("AF", 2);
+  EXPECT_GE(-1, empty.cmp(a));
+  EXPECT_LE(1, a.cmp(empty));
+  EXPECT_GE(-1, a.cmp(ab));
+  EXPECT_LE(1, ab.cmp(a));
+  EXPECT_EQ(0, ab.cmp(ab));
+  EXPECT_GE(-1, ab.cmp(af));
+  EXPECT_LE(1, af.cmp(ab));
+}
+
 TEST(BufferList, zero) {
   //
   // void zero()
-- 
1.7.10.4

Re: [PATCH] buffer::ptr::cmp only compares up to the smallest length

[Sage Weil]

On Sun, 17 Feb 2013, Loic Dachary wrote:
> When running
> 
>   bufferptr a("A", 1);
>   bufferptr ab("AB", 2);
>   a.cmp(ab);
> 
> it returned zero because cmp only compared up to the length of the
> smallest buffer. The tests comparing the length of the buffers are
> moved before the memcmp comparing the actual content of the buffers.
> 
> http://tracker.ceph.com/issues/4170 refs #4170
> 
> Signed-off-by: Loic Dachary <loic@dachary.org>

The problem here is that for B cmp AB, we should be 1, because 'B' > 'A'.  
The length only matters if we reach the end and everything so far is 
equal.

> ---
>  src/common/buffer.cc   |    8 +-------
>  src/test/bufferlist.cc |   14 ++++++++++++++
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/common/buffer.cc b/src/common/buffer.cc
> index e10d6c9..5a88849 100644
> --- a/src/common/buffer.cc
> +++ b/src/common/buffer.cc
> @@ -368,17 +368,11 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
>  
>    int buffer::ptr::cmp(const ptr& o)
>    {
> -    int l = _len < o._len ? _len : o._len;
> -    if (l) {
> -      int r = memcmp(c_str(), o.c_str(), l);
> -      if (!r)
> -	return r;

I think this is the bug.. it should be if (r) return r, and fall through 
below only if r == 0 because the buffers are equal.

sage

> -    }
>      if (_len < o._len)
>        return -1;
>      if (_len > o._len)
>        return 1;
> -    return 0;
> +    return memcmp(c_str(), o.c_str(), _len);
>    }
>  
>    bool buffer::ptr::is_zero() const
> diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
> index 71c2e79..aac41c6 100644
> --- a/src/test/bufferlist.cc
> +++ b/src/test/bufferlist.cc
> @@ -9,6 +9,20 @@
>  
>  #define MAX_TEST 1000000
>  
> +TEST(BufferPtr, cmp) {
> +  bufferptr empty;
> +  bufferptr a("A", 1);
> +  bufferptr ab("AB", 2);
> +  bufferptr af("AF", 2);
> +  EXPECT_GE(-1, empty.cmp(a));
> +  EXPECT_LE(1, a.cmp(empty));
> +  EXPECT_GE(-1, a.cmp(ab));
> +  EXPECT_LE(1, ab.cmp(a));
> +  EXPECT_EQ(0, ab.cmp(ab));
> +  EXPECT_GE(-1, ab.cmp(af));
> +  EXPECT_LE(1, af.cmp(ab));
> +}
> +
>  TEST(BufferList, zero) {
>    //
>    // void zero()
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] buffer::ptr::cmp only compares up to the smallest length

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

Hi Sage,

My bad, indeed. I'll resubmit a patch.

Cheers

On 02/17/2013 03:11 AM, Sage Weil wrote:
> On Sun, 17 Feb 2013, Loic Dachary wrote:
>> When running
>>
>>   bufferptr a("A", 1);
>>   bufferptr ab("AB", 2);
>>   a.cmp(ab);
>>
>> it returned zero because cmp only compared up to the length of the
>> smallest buffer. The tests comparing the length of the buffers are
>> moved before the memcmp comparing the actual content of the buffers.
>>
>> http://tracker.ceph.com/issues/4170 refs #4170
>>
>> Signed-off-by: Loic Dachary <loic@dachary.org>
> 
> The problem here is that for B cmp AB, we should be 1, because 'B' > 'A'.  
> The length only matters if we reach the end and everything so far is 
> equal.
> 
>> ---
>>  src/common/buffer.cc   |    8 +-------
>>  src/test/bufferlist.cc |   14 ++++++++++++++
>>  2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/common/buffer.cc b/src/common/buffer.cc
>> index e10d6c9..5a88849 100644
>> --- a/src/common/buffer.cc
>> +++ b/src/common/buffer.cc
>> @@ -368,17 +368,11 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
>>  
>>    int buffer::ptr::cmp(const ptr& o)
>>    {
>> -    int l = _len < o._len ? _len : o._len;
>> -    if (l) {
>> -      int r = memcmp(c_str(), o.c_str(), l);
>> -      if (!r)
>> -	return r;
> 
> I think this is the bug.. it should be if (r) return r, and fall through 
> below only if r == 0 because the buffers are equal.
> 
> sage
> 
>> -    }
>>      if (_len < o._len)
>>        return -1;
>>      if (_len > o._len)
>>        return 1;
>> -    return 0;
>> +    return memcmp(c_str(), o.c_str(), _len);
>>    }
>>  
>>    bool buffer::ptr::is_zero() const
>> diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
>> index 71c2e79..aac41c6 100644
>> --- a/src/test/bufferlist.cc
>> +++ b/src/test/bufferlist.cc
>> @@ -9,6 +9,20 @@
>>  
>>  #define MAX_TEST 1000000
>>  
>> +TEST(BufferPtr, cmp) {
>> +  bufferptr empty;
>> +  bufferptr a("A", 1);
>> +  bufferptr ab("AB", 2);
>> +  bufferptr af("AF", 2);
>> +  EXPECT_GE(-1, empty.cmp(a));
>> +  EXPECT_LE(1, a.cmp(empty));
>> +  EXPECT_GE(-1, a.cmp(ab));
>> +  EXPECT_LE(1, ab.cmp(a));
>> +  EXPECT_EQ(0, ab.cmp(ab));
>> +  EXPECT_GE(-1, ab.cmp(af));
>> +  EXPECT_LE(1, af.cmp(ab));
>> +}
>> +
>>  TEST(BufferList, zero) {
>>    //
>>    // void zero()
>> -- 
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH] buffer::ptr::cmp only compares up to the smallest length

[Loic Dachary]

When running

  bufferptr a("A", 1);
  bufferptr ab("AB", 2);
  a.cmp(ab);

it returned zero because. cmp only compared up to the length of the
smallest buffer and returned if they are identical. The function is
modified to compare the length of the buffers instead of returning.

http://tracker.ceph.com/issues/4170 refs #4170

Signed-off-by: Loic Dachary <loic@dachary.org>
---
 src/common/buffer.cc   |    2 +-
 src/test/bufferlist.cc |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index e10d6c9..df50cfc 100644
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -371,7 +371,7 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
     int l = _len < o._len ? _len : o._len;
     if (l) {
       int r = memcmp(c_str(), o.c_str(), l);
-      if (!r)
+      if (r)
 	return r;
     }
     if (_len < o._len)
diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 71c2e79..7abced1 100644
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -9,6 +9,23 @@
 
 #define MAX_TEST 1000000
 
+TEST(BufferPtr, cmp) {
+  bufferptr empty;
+  bufferptr a("A", 1);
+  bufferptr ab("AB", 2);
+  bufferptr af("AF", 2);
+  bufferptr acc("ACC", 3);
+  EXPECT_GE(-1, empty.cmp(a));
+  EXPECT_LE(1, a.cmp(empty));
+  EXPECT_GE(-1, a.cmp(ab));
+  EXPECT_LE(1, ab.cmp(a));
+  EXPECT_EQ(0, ab.cmp(ab));
+  EXPECT_GE(-1, ab.cmp(af));
+  EXPECT_LE(1, af.cmp(ab));
+  EXPECT_GE(-1, acc.cmp(af));
+  EXPECT_LE(1, af.cmp(acc));
+}
+
 TEST(BufferList, zero) {
   //
   // void zero()
-- 
1.7.10.4

Re: [PATCH] buffer::ptr::cmp only compares up to the smallest length

[Sage Weil]

Applied, thanks!

I left this in master since the only current caller I see is in the omap 
scrubbing code.  We may want to backport it soonish, though.

sage


On Sun, 17 Feb 2013, Loic Dachary wrote:

> When running
> 
>   bufferptr a("A", 1);
>   bufferptr ab("AB", 2);
>   a.cmp(ab);
> 
> it returned zero because. cmp only compared up to the length of the
> smallest buffer and returned if they are identical. The function is
> modified to compare the length of the buffers instead of returning.
> 
> http://tracker.ceph.com/issues/4170 refs #4170
> 
> Signed-off-by: Loic Dachary <loic@dachary.org>
> ---
>  src/common/buffer.cc   |    2 +-
>  src/test/bufferlist.cc |   17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/src/common/buffer.cc b/src/common/buffer.cc
> index e10d6c9..df50cfc 100644
> --- a/src/common/buffer.cc
> +++ b/src/common/buffer.cc
> @@ -371,7 +371,7 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
>      int l = _len < o._len ? _len : o._len;
>      if (l) {
>        int r = memcmp(c_str(), o.c_str(), l);
> -      if (!r)
> +      if (r)
>  	return r;
>      }
>      if (_len < o._len)
> diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
> index 71c2e79..7abced1 100644
> --- a/src/test/bufferlist.cc
> +++ b/src/test/bufferlist.cc
> @@ -9,6 +9,23 @@
>  
>  #define MAX_TEST 1000000
>  
> +TEST(BufferPtr, cmp) {
> +  bufferptr empty;
> +  bufferptr a("A", 1);
> +  bufferptr ab("AB", 2);
> +  bufferptr af("AF", 2);
> +  bufferptr acc("ACC", 3);
> +  EXPECT_GE(-1, empty.cmp(a));
> +  EXPECT_LE(1, a.cmp(empty));
> +  EXPECT_GE(-1, a.cmp(ab));
> +  EXPECT_LE(1, ab.cmp(a));
> +  EXPECT_EQ(0, ab.cmp(ab));
> +  EXPECT_GE(-1, ab.cmp(af));
> +  EXPECT_LE(1, af.cmp(ab));
> +  EXPECT_GE(-1, acc.cmp(af));
> +  EXPECT_LE(1, af.cmp(acc));
> +}
> +
>  TEST(BufferList, zero) {
>    //
>    // void zero()
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>