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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
--
Loïc Dachary, Artisan Logiciel Libre
Attachment:
signature.asc
Description: OpenPGP digital signature
