rpc_address: remove c interface of dsn_address_t #4

shengofsun · 2018-04-10T02:11:09Z

merge the structure of dsn_address_t to rpc_address.h, make code
less confused

merge the structure of dsn_address_t to rpc_address.h, make code less confused

neverchanje · 2018-04-10T03:07:27Z

include/dsn/tool-api/rpc_address.h

+class rpc_address
+{
+public:
+    static const rpc_address sInvalid;


Why use camel case naming here? May invalid_address be a preferred name?

neverchanje · 2018-04-10T03:11:22Z

include/dsn/tool-api/rpc_address.h

+    }
+    rpc_uri_address *uri_address() const { return (rpc_uri_address *)(uintptr_t)_addr.uri.uri; }
+    bool is_invalid() const { return _addr.v4.type == HOST_TYPE_INVALID; }
+    void set_invalid() { clear(); }


Remove this?

neverchanje · 2018-04-10T03:18:30Z

src/core/core/address.cpp

@@ -57,15 +57,16 @@



Rename this file to "rpc_address.cpp"? Since this file contains only rpc_address, no other class exists.

neverchanje · 2018-04-10T03:31:51Z

src/core/core/address.cpp

+    _addr.group.group = (uint64_t)addr;
+}
+
+rpc_address rpc_address::clone() const


I didn't see any difference between clone and operator=. Even if they are implemented differently, they are supposed to be the same. Remove clone?

clone is used for uri_resolver in the old code, currently it's indeed useless. Let's remove it.

remove redudant clear function, rename sInvalid to s_invalid_address

neverchanje · 2018-04-10T05:52:04Z

src/core/core/rpc_message.cpp

@@ -214,6 +214,13 @@ message_ex::message_ex()

 message_ex::~message_ex()
 {
+    // when recv a request, message_header is hidden ahead of buffer(see@create_receive_message


I don't understand what does "hidden ahead of buffer" mean? Could you make it clear?
BTW, don't use abbreviation like "recv" in your comment. Always be careful of your grammar.

neverchanje · 2018-04-10T05:58:20Z

src/core/core/address.cpp


 namespace dsn {
-const rpc_address rpc_group_address::_invalid;
-}
+const rpc_address rpc_address::sInvalid;

 #ifdef _WIN32


We should no longer support win32. See L71, L247, L256 and L36 of this file, L81 of src/core/tests/address.cpp.

qinzuoyan · 2018-04-10T10:31:40Z

src/core/core/rpc_message.cpp

+    // then new request's header will be invalid.
+    //
+    // so we don't release_buffer_header().
+    // however, this won't lead to any memory leak problems as long as the header is a POD type


这里说"the header is a POD type"

注释里都写了让看message_header的Attention了……
改倒是可以改。但是，我就是看着留洋的注释搞明白的，没觉得哪里有容易不明白的地方……

还是写清楚点吧：however, this won't lead to any memory leak problems as long as the header can be thought of as POD type (because the from_address must be IPv4 address), refer to the ATTENTION of message_header.

后来这个地方改过了，你看看新的注释，应该也能说清楚了

qinzuoyan · 2018-04-10T10:32:37Z

src/core/core/rpc_message.cpp

+    // however, this won't lead to any memory leak problems as long as the header is a POD type
+    // see@ Attention of message_header
+
+    // release_buffer_header();


这个函数没有真正被使用过，所以只是为了帮助写注释？

qinzuoyan · 2018-04-10T10:33:23Z

src/core/core/rpc_message.cpp

    header = (message_header *)ptr;
 }

+void message_ex::release_buffer_header()
+{
+    // message_header contain variable that is not POD-type, so we call these variable's destructor


这里也说“message_header contain variable that is not POD-type”

qinzuoyan · 2018-04-10T10:34:20Z

src/core/core/rpc_message.cpp

@@ -465,9 +478,18 @@ void message_ex::prepare_buffer_header()
    this->_rw_offset = (int)sizeof(message_header);
    this->buffers.push_back(buffer);

+    // here, we should call placement new, because message_header contain variable that is not


这里又说“message_header contain variable that is not POD-type”，与上面的注释不一致，容易confused

shengofsun · 2018-04-10T13:51:54Z

@qinzuoyan comments are fixed

qinzuoyan · 2018-04-10T13:52:50Z

@shengofsun more comments.

neverchanje · 2018-04-10T14:54:29Z

src/core/core/rpc_message.cpp

    header = (message_header *)ptr;
 }

+void message_ex::release_buffer_header()


Remove this, I don't see any place using it.

当前用这种方式注释这段代码没问题，你有了placement new，当然得有显式的析构函数。不加这段代码才容易让人感到疑惑

neverchanje · 2018-04-10T15:50:23Z

src/core/core/rpc_message.cpp

@@ -214,6 +214,19 @@ message_ex::message_ex()

 message_ex::~message_ex()
 {
+    // when receiving a request, the lifetime of message_header object is managed by


建议把这段注释全删了

把 copy_and_prepare_send 的 clone_content 参数也删了，我搜了一下没有哪里有用除了 false 以外的参数。并且改名叫 forward_to_send，因为它只在 rpc_engine::forward 里用。

加以下注释在 message_ex::buffers 上面：

// For sending, // buffers[0] = (memory of message_header), // buffers[1] = (memory of request content). // // For receiving, // buffers[0] = (memory of message content), // (memory of message_header) = buffers[0] - sizeof(memory_header). // // In the case where request is forwarded from one server to another, the received request // as well as its message header will be entirely moved into the new request to send. // \see message_ex::copy_and_prepare_send and rpc_engine::forward

原来底下这段注释的目的是解释 message_header 使用 dsn::blob 来共享内存的原因，但如果只是为了 forward，我没有理解为什么还要用 blob？可不可以用 unique_ptr<char[]>?

再提个pr来改吧，这个pr是rpc_address相关的，以及由于rpc_address改动而对潜在问题的相关改动。
在message上再纠结下去，超纲了。

qinzuoyan · 2018-04-11T05:46:51Z

LGTM

rpc_address: remove c interface of dsn_address_t

7f543c4

merge the structure of dsn_address_t to rpc_address.h, make code less confused

shengofsun requested review from qinzuoyan, neverchanje and LoveHeat April 10, 2018 02:11

neverchanje reviewed Apr 10, 2018

View reviewed changes

rpc_address: small code refactor

82e5a89

remove redudant clear function, rename sInvalid to s_invalid_address

neverchanje reviewed Apr 10, 2018

View reviewed changes

shengofsun added 2 commits April 10, 2018 14:11

rpc_address: remove clone, rename address.cpp

ba089ac

rpc_address: remove useless WIN32 macros. rpc_message: fix comments

dd7d4e9

neverchanje approved these changes Apr 10, 2018

View reviewed changes

rpc_address: fix comments

4b535d3

qinzuoyan reviewed Apr 10, 2018

View reviewed changes

rpc_message: fix comments about message header

16c0d16

neverchanje reviewed Apr 10, 2018

View reviewed changes

rpc_message: fix comments

46a98c2

qinzuoyan approved these changes Apr 11, 2018

View reviewed changes

shengofsun merged commit 7b84a6e into XiaoMi:master Apr 11, 2018

qinzuoyan mentioned this pull request Aug 20, 2018

代码重构与化简 #141

Open

28 tasks

foreverneverer mentioned this pull request Jan 10, 2020

ASAN: heap-buffer-overflow in meta_test_base.h #376

Closed

ZhongChaoqiang mentioned this pull request Apr 27, 2022

fix: crash when huge write #1099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc_address: remove c interface of dsn_address_t #4

rpc_address: remove c interface of dsn_address_t #4

shengofsun commented Apr 10, 2018

neverchanje Apr 10, 2018

neverchanje Apr 10, 2018

neverchanje Apr 10, 2018 •

edited

neverchanje Apr 10, 2018

shengofsun Apr 10, 2018

neverchanje Apr 10, 2018

shengofsun Apr 10, 2018

neverchanje Apr 10, 2018

qinzuoyan Apr 10, 2018

shengofsun Apr 10, 2018

qinzuoyan Apr 10, 2018

shengofsun Apr 10, 2018

qinzuoyan Apr 10, 2018

shengofsun Apr 10, 2018

qinzuoyan Apr 10, 2018

qinzuoyan Apr 10, 2018

shengofsun commented Apr 10, 2018

qinzuoyan commented Apr 10, 2018

neverchanje Apr 10, 2018

shengofsun Apr 11, 2018

neverchanje Apr 10, 2018 •

edited

shengofsun Apr 11, 2018

qinzuoyan commented Apr 11, 2018

rpc_address: remove c interface of dsn_address_t #4

rpc_address: remove c interface of dsn_address_t #4

Conversation

shengofsun commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neverchanje Apr 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shengofsun commented Apr 10, 2018

qinzuoyan commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neverchanje Apr 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinzuoyan commented Apr 11, 2018

neverchanje Apr 10, 2018 •

edited

neverchanje Apr 10, 2018 •

edited