client: fix segment fault in SourceReader #358
Conversation
wu-hanqing
force-pushed
the
fix/source-reader-segv
branch
2 times, most recently
from
75c0f8a to
5f04650
Compare
| struct LeaseRefreshResult; | ||
|
|
||
| // MDSClient是client与MDS通信的唯一窗口 | ||
| class MDSClient { | ||
| class MDSClient : public MDSClientBase, |
There was a problem hiding this comment.
Why dose ‘MDSClient’ change to inherit from 'MDSClientBase' ?
There was a problem hiding this comment.
After removing the option from MDSClientBase, it becomes an empty class. So if we define a member variable of class MDSClientBase, it at least occupies 1-byte memory. To avoid taking up extra space, inherit it is a good solution. Also, make MDSClient inherits from MDSClientBase has no bad effect.
src/client/mds_client_base.h
Outdated
| @@ -100,7 +96,7 @@ class MDSClientBase { | |||
| * @param: metaServerOpt为mdsclient的配置信息 | |||
There was a problem hiding this comment.
change the function description
There was a problem hiding this comment.
Now, this function is useless, so I just delete it.
src/client/mds_client.cpp
Outdated
| @@ -81,20 +63,28 @@ LIBCURVE_ERROR MDSClient::Initialize(const MetaServerOption& metaServerOpt) { | |||
|
|
|||
| metaServerOpt_ = metaServerOpt; | |||
|
|
|||
| int rc = mdsClientBase_.Init(metaServerOpt_); | |||
| int rc = MDSClientBase::Init(); | |||
There was a problem hiding this comment.
MDSClientBase::Init() now is return 0 directly, Whether this judgment is needed？
| @@ -200,8 +201,7 @@ struct MDSClientMetric { | |||
| explicit MDSClientMetric(const std::string& prefix_ = "") | |||
| : prefix(!prefix_.empty() | |||
| ? prefix_ | |||
| : "curve_mds_client_" + | |||
| std::to_string(reinterpret_cast<uint64_t>(this))), | |||
| : "curve_mds_client_" + common::ToHexString(this)), | |||
There was a problem hiding this comment.
why change to hexString? give an example for prefix
There was a problem hiding this comment.
The prefix in here is just to distinguish different MDSClientMetric's instances because for a specific name, bvar only expose once. So, I just add the instance's address in the prefix, and change the address to hex string is more obvious.
wu-hanqing
force-pushed
the
fix/source-reader-segv
branch
from
5f04650 to
e18b48f
Compare
After opencurve#172 and opencurve#209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <[email protected]>
wu-hanqing
force-pushed
the
fix/source-reader-segv
branch
from
e18b48f to
cfcc912
Compare
add k8s scheduler GSoC 2021 entry
What problem does this PR solve?
Problem Summary:
After #172 and #209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but
FileInstance::mdsclient_points toFileClient::mdsClient_, so after FileClient is destroyed,FileInstance::mdsclient_becomes dangling pointer.What is changed and how it works?
What's Changed:
To fix this problem, we let
FileClient::mdsClient_be a shared_ptr, and each FileInstance holds ownership of it.Side effects(Breaking backward compatibility? Performance regression?):
Check List