New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common/buffer: close pipe fd if set nonblocking fails. #12828

Merged
merged 1 commit into from Jan 23, 2017

Conversation

Projects
None yet
2 participants
@donglinpeng
Contributor

donglinpeng commented Jan 9, 2017

Signed-off-by: donglinpeng donglinpeng@cmss.chinamobile.com

@@ -540,6 +540,7 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER;
if (r < 0) {
bdout << "raw_pipe: error setting nonblocking flag on temp pipe: "
<< cpp_strerror(r) << bendl;
close_pipe(tmpfd);

This comment has been minimized.

@tchaikov

tchaikov Jan 9, 2017

Contributor

might want to consolidate all the "close_pipe()` calls in the exit path to a single one with https://github.com/ceph/ceph/blob/master/src/include/scope_guard.h .

This comment has been minimized.

@donglinpeng

donglinpeng Jan 9, 2017

Contributor

Nice. I will resubmit code after building test with scope_guard.h.

This comment has been minimized.

@donglinpeng

donglinpeng Jan 11, 2017

Contributor

Hi, I try to consolidate all the "close_pipe()" calls in the exit path to a single one with scope_guard.h, but I encountered one test case failure. I am not sure how to use scope_guard.h. Could you give me some suggests, thank you.

@tchaikov tchaikov added the common label Jan 9, 2017

@tchaikov tchaikov self-assigned this Jan 11, 2017

@tchaikov tchaikov changed the title from common/buffer:Pipe fd is not closed when set nonblocking fails. to common/buffer: close pipe fd if set nonblocking fails. Jan 17, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 17, 2017

retest this please.

@@ -536,6 +537,7 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER;
<< bendl;
throw error_code(r);
}
auto sg = make_scope_guard([&] { close_pipe(tmpfd); });

This comment has been minimized.

@tchaikov

tchaikov Jan 17, 2017

Contributor

you change should work. but i'd suggest not capture this if not necessary. maybe we can mark close_pipe() a static method, like:

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index 7ca3a66d29..5113e8e5d8 100644
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -515,7 +515,7 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER;
       return 0;
     }

-    void close_pipe(int *fds) {
+    static void close_pipe(const int *fds) {
       if (fds[0] >= 0)
        VOID_TEMP_FAILURE_RETRY(::close(fds[0]));
       if (fds[1] >= 0)
@@ -537,7 +537,7 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER;
              << bendl;
        throw error_code(r);
       }
-      auto sg = make_scope_guard([&] { close_pipe(tmpfd); });
+      auto sg = make_scope_guard([=] { close_pipe(tmpfd); });
       r = set_nonblocking(tmpfd);
       if (r < 0) {
        bdout << "raw_pipe: error setting nonblocking flag on temp pipe: "

This comment has been minimized.

@donglinpeng

donglinpeng Jan 17, 2017

Contributor

Thank you very much. I have resubmitted codes according to you advice.

donglinpeng
common/buffer:Pipe fd is not closed when set nonblocking fails.
Signed-off-by: donglinpeng <donglinpeng@cmss.chinamobile.com>

@tchaikov tchaikov merged commit d54f75d into ceph:master Jan 23, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@donglinpeng donglinpeng deleted the donglinpeng:dlppull branch Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment