Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

*: expose disk_file that was hidden under dsn_handle_t #172

Merged
merged 10 commits into from
Oct 16, 2018

Conversation

neverchanje
Copy link
Contributor

@neverchanje neverchanje commented Oct 12, 2018

This PR also fixes 2 memory leaks:

  • src/dist/nfs/nfs_server_impl.cpp: value of nfs_service_impl::_handles_map is a raw pointer but not deleted during destruction of nfs_service_impl.

  • src/core/tools/common/native_aio_provider.linux.cpp: there's a std::thread wild pointer

@@ -276,6 +276,11 @@ typedef struct
int size;
} dsn_file_buffer_t;

namespace dsn {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果可以的话,你看看能不能把file相关的接口都专门拿出来放到一个头文件里面去。
这样只移除一个dsn_handle_t, 又加个namespace,感觉和这个文件的整体风格很不搭

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个可不可以之后再干,我感觉 disk engine 这个模块都可以放到 utility 下,跟 utility/filesystem.h 合到一起,就是需要整,我最近要专注搞 valgrind

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

主要就是这个不太搭的修改放在这里会有些诡异,因为这个不完整的改动可能会一放就是很久……

专门搞valgrind没问题,集中只提valgrind相关的pr就好了。

另外disk_engine还是别放utility下了。utility/filesystem.h是对OS文件系统API的封装,而rdsn的disk engine是和task相关的异步接口,挪出去不合适。

Copy link
Contributor Author

@neverchanje neverchanje Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那可以改

@@ -80,12 +80,11 @@ void nfs_service_impl::on_copy(const ::dsn::service::copy_request &request,
hfile = dsn_file_open(file_path.c_str(), O_RDONLY | O_BINARY, 0);
if (hfile) {

file_handle_info_on_server *fh = new file_handle_info_on_server;
auto fh = std::make_shared<file_handle_info_on_server>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个地方好像不是个bug?最后delete了

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

只有 close_file() 的时候会 delete,然而_handle_maps 在析构的时候可能不是所有的文件都 close 了,所以要么在 nfs_service_impl 析构的时候 for delete 一下,要么直接用 shared ptr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

了解

@neverchanje neverchanje added the area/memcheck PR or issues related to valgrind memcheck label Oct 15, 2018
@neverchanje
Copy link
Contributor Author

@shengofsun 按你要求改了,可以看下

@neverchanje neverchanje mentioned this pull request Oct 15, 2018
9 tasks
\param offset offset in the file to start reading
\param cb callback aio task to be executed on completion
*/
extern void dsn_file_read(disk_file *file, char *buffer, int count, uint64_t offset, aio_task *cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个dsn_file_read、dsn_file_wrte和dsn_file_write_vector这三个c接口可以和async_calls.h中file namespace下的几个接口整合一下。这样我们就不用保留两套异步的文件接口了。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这一改改的有点大,我怕扯到蛋...我尽量看看

@shengofsun
Copy link
Contributor

另外催人给我过一下hpc_lock的那个review?

@neverchanje
Copy link
Contributor Author

恩,明天我催下

@neverchanje
Copy link
Contributor Author

neverchanje commented Oct 16, 2018

@shengofsun 按你要求又改了,你看看是不是你想要的
麻烦 commit by commit 的看下 30d125f, 6e96a8f 这俩提交,多谢~

@qinzuoyan qinzuoyan merged commit 6d43355 into XiaoMi:master Oct 16, 2018
vagetablechicken pushed a commit to vagetablechicken/rdsn that referenced this pull request Nov 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/memcheck PR or issues related to valgrind memcheck
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants