RadosClient: result code may overflow #6489
Conversation
|
@xiexingguo i am not sure this is the right fix. probably we should deprecate these two methods and have new methods overloading them, for example: int pool_requires_alignment(int64_t pool_id, bool *requires);by returning the error code, and pass a pointer for the "[out]" parameter? |
|
and your |
xiexingguo
force-pushed
the
xxg-wip-13715
branch
from
be1f2ef
to
f2cb664
Compare
|
Agree on fixing the function prototypes to have a return code separate from the output arguments. |
|
@tchaikov I like the new function idea since it's safer. However, to preserve API compatibility, the new function would need to be named: |
xiexingguo
force-pushed
the
xxg-wip-13715
branch
2 times, most recently
from
3c06b09
to
031bee2
Compare
|
@xiexingguo please ignore the false negative from the bot |
pool_requires_alignment and pool_required_alignment may produce overflowed results and need to refactered. Fixes: ceph#13715 Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
xiexingguo
force-pushed
the
xxg-wip-13715
branch
from
b59ad80
to
1f85545
Compare
| objecter->put_osdmap_read(); | ||
| return 0; | ||
| } | ||
|
|
||
| uint64_t librados::RadosClient::pool_required_alignment(int64_t pool_id) | ||
| { | ||
| int r = wait_for_osdmap(); |
There was a problem hiding this comment.
@xiexingguo could you reimplement the deprecated methods using the new ones in another commit in this pr? so we don't less duplicated code and the logic reveals the problem of the old methods in a more visible way.
There was a problem hiding this comment.
@tchaikov Sorry, I don't quite understand what do you mean. Can you make an example?
There was a problem hiding this comment.
diff --git a/src/librados/RadosClient.cc b/src/librados/RadosClient.cc
index a01ae7d..6157e35 100644
--- a/src/librados/RadosClient.cc
+++ b/src/librados/RadosClient.cc
@@ -108,16 +108,13 @@ bool librados::RadosClient::pool_requires_alignment(int64_t pool_id)
uint64_t librados::RadosClient::pool_required_alignment(int64_t pool_id)
{
- int r = wait_for_osdmap();
+ uint64_t alignment = 0;
+ int r = pool_required_alignment2(pool_id, &alignment);
if (r < 0) {
return r;
+ } else {
+ return alignment;
}
-
- const OSDMap *osdmap = objecter->get_osdmap_read();
- uint64_t ret = osdmap->have_pg_pool(pool_id) ?
- osdmap->get_pg_pool(pool_id)->required_alignment() : 0;
- objecter->put_osdmap_read();
- return ret;
}
There was a problem hiding this comment.
@tchaikov Got it. I will do it now.
|
@tchaikov UT passed, please review. |
|
lgtm. |
|
@liewegas Hi, sage, could you please add this one for test too? |
| uint64_t alignment; | ||
| int r = pool_required_alignment2(pool_id, &alignment); | ||
| if (r < 0) { | ||
| return r; |
There was a problem hiding this comment.
One problem here. Return a negative int as uint64_t. What does caller do with a huge number? Should it return 0 and caller should check for that as an error indication?
There was a problem hiding this comment.
I see that is why there is a new interface, but I think returning 0 on error for the original interface is reasonable.
xiexingguo
force-pushed
the
xxg-wip-13715
branch
from
43af20e
to
3656007
Compare
|
@dzafman Fixup as you suggested. Please review. |
|
lgtm |
xiexingguo
force-pushed
the
xxg-wip-13715
branch
2 times, most recently
from
a5a9e35
to
1f85545
Compare
so we don't less duplicated code and the logic reveals the problem of the old methods in a more visible way. Fixes: ceph#13715 Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
librados: fix pool alignment API overflow issue Reviewed-by: Kefu Chai <kchai@redhat.com> Reviewed-by: David Zafman <dzafman@redhat.com>
For more details about this issue, check http://tracker.ceph.com/issues/13715