You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/11/14 12:19:15 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1238: refactor: use linux_fd_t instead of dsn_handle_t for file descriptor
empiredan commented on code in PR #1238:
URL: https://github.com/apache/incubator-pegasus/pull/1238#discussion_r1021435479
##########
src/aio/disk_engine.cpp:
##########
@@ -91,7 +91,7 @@ aio_task *disk_write_queue::unlink_next_workload(void *plength)
return first;
}
-disk_file::disk_file(dsn_handle_t handle) : _handle(handle) {}
+disk_file::disk_file(linux_fd_t fd) : _fd(fd.fd) {}
Review Comment:
Use copy constructor `_fd(fd)` instead ?
##########
src/nfs/nfs_server_impl.cpp:
##########
@@ -76,26 +77,23 @@ void nfs_service_impl::on_copy(const ::dsn::service::copy_request &request,
std::string file_path =
dsn::utils::filesystem::path_combine(request.source_dir, request.file_name);
- disk_file *hfile;
+ disk_file *dfile = nullptr;
{
zauto_lock l(_handles_map_lock);
auto it = _handles_map.find(file_path); // find file handle cache first
- if (it == _handles_map.end()) // not found
- {
- hfile = file::open(file_path.c_str(), O_RDONLY | O_BINARY, 0);
- if (hfile) {
-
+ if (it == _handles_map.end()) {
+ dfile = file::open(file_path.c_str(), O_RDONLY | O_BINARY, 0);
+ if (dfile) {
Review Comment:
```suggestion
if (dfile != nullptr) {
```
##########
src/aio/native_linux_aio_provider.cpp:
##########
@@ -42,33 +42,33 @@ native_linux_aio_provider::native_linux_aio_provider(disk_engine *disk) : aio_pr
native_linux_aio_provider::~native_linux_aio_provider() {}
-dsn_handle_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
+linux_fd_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
{
- dsn_handle_t fh = (dsn_handle_t)(uintptr_t)::open(file_name, flag, pmode);
- if (fh == DSN_INVALID_FILE_HANDLE) {
+ int fd = ::open(file_name, flag, pmode);
Review Comment:
```suggestion
auto fd = ::open(file_name, flag, pmode);
```
##########
src/aio/native_linux_aio_provider.cpp:
##########
@@ -42,33 +42,33 @@ native_linux_aio_provider::native_linux_aio_provider(disk_engine *disk) : aio_pr
native_linux_aio_provider::~native_linux_aio_provider() {}
-dsn_handle_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
+linux_fd_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
{
- dsn_handle_t fh = (dsn_handle_t)(uintptr_t)::open(file_name, flag, pmode);
- if (fh == DSN_INVALID_FILE_HANDLE) {
+ int fd = ::open(file_name, flag, pmode);
+ if (fd == DSN_INVALID_FILE_HANDLE) {
LOG_ERROR("create file failed, err = %s", strerror(errno));
}
- return fh;
+ return linux_fd_t(fd);
}
-error_code native_linux_aio_provider::close(dsn_handle_t fh)
+error_code native_linux_aio_provider::close(linux_fd_t fd)
{
- if (fh == DSN_INVALID_FILE_HANDLE || ::close((int)(uintptr_t)(fh)) == 0) {
+ if (fd.is_invalid() || ::close(fd.fd) == 0) {
return ERR_OK;
- } else {
- LOG_ERROR("close file failed, err = %s", strerror(errno));
- return ERR_FILE_OPERATION_FAILED;
}
+
+ LOG_ERROR_F("close file failed, err = {}", strerror(errno));
Review Comment:
```suggestion
LOG_ERROR_F("close file failed, err = {}", utils::safe_strerror(errno));
```
##########
src/aio/native_linux_aio_provider.cpp:
##########
@@ -42,33 +42,33 @@ native_linux_aio_provider::native_linux_aio_provider(disk_engine *disk) : aio_pr
native_linux_aio_provider::~native_linux_aio_provider() {}
-dsn_handle_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
+linux_fd_t native_linux_aio_provider::open(const char *file_name, int flag, int pmode)
{
- dsn_handle_t fh = (dsn_handle_t)(uintptr_t)::open(file_name, flag, pmode);
- if (fh == DSN_INVALID_FILE_HANDLE) {
+ int fd = ::open(file_name, flag, pmode);
+ if (fd == DSN_INVALID_FILE_HANDLE) {
LOG_ERROR("create file failed, err = %s", strerror(errno));
}
- return fh;
+ return linux_fd_t(fd);
}
-error_code native_linux_aio_provider::close(dsn_handle_t fh)
+error_code native_linux_aio_provider::close(linux_fd_t fd)
{
- if (fh == DSN_INVALID_FILE_HANDLE || ::close((int)(uintptr_t)(fh)) == 0) {
+ if (fd.is_invalid() || ::close(fd.fd) == 0) {
return ERR_OK;
- } else {
- LOG_ERROR("close file failed, err = %s", strerror(errno));
- return ERR_FILE_OPERATION_FAILED;
}
+
+ LOG_ERROR_F("close file failed, err = {}", strerror(errno));
+ return ERR_FILE_OPERATION_FAILED;
}
-error_code native_linux_aio_provider::flush(dsn_handle_t fh)
+error_code native_linux_aio_provider::flush(linux_fd_t fd)
{
- if (fh == DSN_INVALID_FILE_HANDLE || ::fsync((int)(uintptr_t)(fh)) == 0) {
+ if (fd.is_invalid() || ::fsync(fd.fd) == 0) {
return ERR_OK;
- } else {
- LOG_ERROR("flush file failed, err = %s", strerror(errno));
- return ERR_FILE_OPERATION_FAILED;
}
+
+ LOG_ERROR_F("flush file failed, err = {}", strerror(errno));
Review Comment:
```suggestion
LOG_ERROR_F("flush file failed, err = {}", utils::safe_strerror(errno));
```
##########
src/aio/native_linux_aio_provider.cpp:
##########
@@ -117,7 +117,7 @@ error_code native_linux_aio_provider::write(const aio_context &aio_ctx,
error_code native_linux_aio_provider::read(const aio_context &aio_ctx,
/*out*/ uint64_t *processed_bytes)
{
- ssize_t ret = pread(static_cast<int>((ssize_t)aio_ctx.file),
+ ssize_t ret = pread(aio_ctx.dfile->native_handle().fd,
Review Comment:
```suggestion
auto ret = pread(aio_ctx.dfile->native_handle().fd,
```
##########
src/aio/disk_engine.cpp:
##########
@@ -256,9 +250,9 @@ void disk_engine::complete_io(aio_task *aio, error_code err, uint64_t bytes)
// no batching
else {
- auto df = (disk_file *)(aio->get_aio_context()->file_object);
+ auto dfile = aio->get_aio_context()->dfile;
if (aio->get_aio_context()->type == AIO_Read) {
- auto wk = df->on_read_completed(aio, err, (size_t)bytes);
+ auto wk = dfile->on_read_completed(aio, err, (size_t)bytes);
if (wk) {
Review Comment:
```suggestion
if (wk != nullptr) {
```
##########
src/aio/file_io.cpp:
##########
@@ -32,12 +32,12 @@ namespace file {
/*extern*/ disk_file *open(const char *file_name, int flag, int pmode)
{
- dsn_handle_t nh = disk_engine::provider().open(file_name, flag, pmode);
- if (nh != DSN_INVALID_FILE_HANDLE) {
- return new disk_file(nh);
- } else {
+ linux_fd_t fd = disk_engine::provider().open(file_name, flag, pmode);
Review Comment:
```suggestion
auto fd = disk_engine::provider().open(file_name, flag, pmode);
```
##########
src/aio/disk_engine.cpp:
##########
@@ -267,7 +261,7 @@ void disk_engine::complete_io(aio_task *aio, error_code err, uint64_t bytes)
// write
else {
uint64_t sz;
- auto wk = df->on_write_completed(aio, (void *)&sz, err, (size_t)bytes);
+ auto wk = dfile->on_write_completed(aio, (void *)&sz, err, (size_t)bytes);
if (wk) {
Review Comment:
```suggestion
if (wk != nullptr) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org