Re: compiling stops at od compare

Commit Message

Willem Jan Withagen Jan. 31, 2016, 12:25 p.m. UTC

On 31-1-2016 00:13, Willem Jan Withagen wrote:
> On 30-1-2016 23:57, Matt Benjamin wrote:
>> Should we use std::min here (that might require a cast, iirc)?
> 
> Well the most important issue I have:
> 	while(len>0)
> where len is of type size_t has only exactly one chance to be true, aka
> len = 0. negative numbers do not exist.
> 
> So casting it to int is a bad(tm) thing.
> 
> But as I haven't designed the code, i can only react to the compiler
> error and analyze it. And then my conclusion is that the cast can only
> increase the chance on an error. And thus the compiler is correct in
> triggering.

Sorry,

I did not answer your question.

If using std::min requires a cast, then it will fall in the same pitfall
as the current code does. if there is a std::min version that does not
autocast/promote size_t to int it might work as well.
Using MIN does "the right thing", without the cast.


This does the job on Centos 7 for me....

--WjW

>> ----- Original Message -----
>>> From: "Willem Jan Withagen" <[e-mail]>
>>> To: [e-mail]
>>> Sent: Saturday, January 30, 2016 8:22:07 AM
>>> Subject: compiling stops at od compare
>>>
>>> When trying to compile on CentOS 7, gcc = 4.8.3
>>>
>>> os/bluestore/BlueFS.cc: In member function ‘int
>>> BlueFS::_read(BlueFS::FileReader*, BlueFS::FileReaderBuffer*, uint64_t,
>>> size_t, ceph::bufferlist*, char*)’:
>>> os/bluestore/BlueFS.cc:731:31: warning: comparison between signed and
>>> unsigned integer expressions [-Wsign-compare]
>>>      int r = MIN((int)len, left);
>>>
>>> This MIN is used to determine the amount of buffer that is still left
>>> to be filed.
>>> And here len and left are both size_t..., suggesting that both cannot be
>>> negative. So either both need to be promoted/cast, or neither.
>>>
>>> The cast (int)len suggests that len could be negative.
>>> The part where that could happen is at line 750:
>>>
>>>    off += r;
>>>     len -= r;
>>>     ret += r;
>>>
>>> So there the loop exit needs len to be exactly equal to r. Even if the
>>> loop specifies while(len>0). if len gets "negative" it grows into
>>> something rather big.
>>>
>>> Now if len never gets negative then it also does not need to get cast to
>>> int. If it does, then in the unsigned case it will always be larger than
>>> left.
>>>
>>> So bottomline is that the cast serves no purpose?
>>> Removing it fixes compilation.
>>>
>>> --WjW
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>> the body of a message to [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
index 1bad6a2..13555c9 100644
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -728,7 +728,7 @@  int BlueFS::_read(
     left = buf->get_buf_remaining(off);
     dout(20) << __func__ << " left " << left << " len " << len << dendl;

-    int r = MIN((int)len, left);
+    int r = MIN(len, left);
     if (outbl) {
       bufferlist t;
       t.substr_of(buf->bl, off - buf->bl_off, r);