6:15 a.m.
For each nbd_(unlocked_)?aio_is_* function, split it into an internal
function that tests the state and a public visible API function.
For calls within the library, use the internal function.
This is simple refactoring.
As a side effect this fixes a race condition identified by Eric Blake:
Thread A Thread B
(in a call that holds h->lock) (calling nbd_aio_pread)
--------------------------------------------------------------------
h->state is processing
checks nbd_aio_is_ready
(it's false)
h->state is moved to READY
checks nbd_aio_is_processing
(it's false)
validation check fails
(However the state was valid so the validation check should have
succeeded).
Fixes commit e63a11736930c381a79a8cc2d03844cfff5db3ef.
Thanks: Eric Blake for identifying the race condition above.
---
generator/generator | 15 ++++-----
lib/connect.c | 8 ++---
lib/disconnect.c | 9 +++---
lib/internal.h | 8 +++++
lib/is-state.c | 74 ++++++++++++++++++++++++++++++++++-----------
lib/rw.c | 4 +--
6 files changed, 82 insertions(+), 36 deletions(-)
diff --git a/generator/generator b/generator/generator
index cdf86e4..8ed0a06 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2841,19 +2841,20 @@ let generate_lib_api_c () =
pr " /* We can check the state outside the handle lock because the\n";
pr " * the state is atomic.\n";
pr " */\n";
+ pr " enum state state = h->state;\n";
let tests =
List.map (
function
- | Created -> "nbd_aio_is_created (h)"
- | Connecting -> "nbd_aio_is_connecting (h)"
- | Connected -> "nbd_aio_is_ready (h) || nbd_aio_is_processing
(h)"
- | Closed -> "nbd_aio_is_closed (h)"
- | Dead -> "nbd_aio_is_dead (h)"
+ | Created -> "nbd_internal_is_state_created (state)"
+ | Connecting -> "nbd_internal_is_state_connecting (state)"
+ | Connected -> "nbd_internal_is_state_ready (state) ||
nbd_internal_is_state_processing (state)"
+ | Closed -> "nbd_internal_is_state_closed (state)"
+ | Dead -> "nbd_internal_is_state_dead (state)"
) permitted_states in
pr " if (!(%s)) {\n" (String.concat " ||\n " tests);
- pr " set_error (nbd_aio_is_created (h) ? ENOTCONN : EINVAL,\n";
+ pr " set_error (nbd_internal_is_state_created (state) ? ENOTCONN :
EINVAL,\n";
pr " \"invalid state: %%s: the handle must be
%%s\",\n";
- pr " nbd_internal_state_short_string (h->state),\n";
+ pr " nbd_internal_state_short_string (state),\n";
pr " \"%s\");\n" (permitted_state_text
permitted_states);
pr " return %s;\n" errcode;
pr " }\n";
diff --git a/lib/connect.c b/lib/connect.c
index 50ec58a..63d2234 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -38,16 +38,16 @@
static int
error_unless_ready (struct nbd_handle *h)
{
- if (nbd_unlocked_aio_is_ready (h))
+ if (nbd_internal_is_state_ready (h->state))
return 0;
/* Why did it fail? */
- if (nbd_unlocked_aio_is_closed (h)) {
+ if (nbd_internal_is_state_closed (h->state)) {
set_error (0, "connection is closed");
return -1;
}
- if (nbd_unlocked_aio_is_dead (h))
+ if (nbd_internal_is_state_dead (h->state))
/* Don't set the error here, keep the error set when
* the connection died.
*/
@@ -62,7 +62,7 @@ error_unless_ready (struct nbd_handle *h)
static int
wait_until_connected (struct nbd_handle *h)
{
- while (nbd_unlocked_aio_is_connecting (h)) {
+ while (nbd_internal_is_state_connecting (h->state)) {
if (nbd_unlocked_poll (h, -1) == -1)
return -1;
}
diff --git a/lib/disconnect.c b/lib/disconnect.c
index 6695e59..355a1c9 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -29,15 +29,14 @@
int
nbd_unlocked_shutdown (struct nbd_handle *h)
{
-
- if (nbd_unlocked_aio_is_ready (h) ||
- nbd_unlocked_aio_is_processing (h)) {
+ if (nbd_internal_is_state_ready (h->state) ||
+ nbd_internal_is_state_processing (h->state)) {
if (nbd_unlocked_aio_disconnect (h, 0) == -1)
return -1;
}
- while (!nbd_unlocked_aio_is_closed (h) &&
- !nbd_unlocked_aio_is_dead (h)) {
+ while (!nbd_internal_is_state_closed (h->state) &&
+ !nbd_internal_is_state_dead (h->state)) {
if (nbd_unlocked_poll (h, -1) == -1)
return -1;
}
diff --git a/lib/internal.h b/lib/internal.h
index c1a57ac..691a1eb 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -264,6 +264,14 @@ extern int nbd_internal_set_size_and_flags (struct nbd_handle *h,
uint64_t exportsize,
uint16_t eflags);
+/* is-state.c */
+extern bool nbd_internal_is_state_created (enum state state);
+extern bool nbd_internal_is_state_connecting (enum state state);
+extern bool nbd_internal_is_state_ready (enum state state);
+extern bool nbd_internal_is_state_processing (enum state state);
+extern bool nbd_internal_is_state_dead (enum state state);
+extern bool nbd_internal_is_state_closed (enum state state);
+
/* protocol.c */
extern int nbd_internal_errno_of_nbd_error (uint32_t error);
extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type);
diff --git a/lib/is-state.c b/lib/is-state.c
index 5ed2ee9..55d103b 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -26,11 +26,12 @@
#include "internal.h"
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_created (struct nbd_handle *h)
+/* Internal functions to test state or groups of states. */
+
+bool
+nbd_internal_is_state_created (enum state state)
{
- return h->state == STATE_START;
+ return state == STATE_START;
}
static int
@@ -51,20 +52,18 @@ is_connecting_group (enum state_group group)
}
}
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
+bool
+nbd_internal_is_state_connecting (enum state state)
{
- enum state_group group = nbd_internal_state_group (h->state);
+ enum state_group group = nbd_internal_state_group (state);
return is_connecting_group (group);
}
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_ready (struct nbd_handle *h)
+bool
+nbd_internal_is_state_ready (enum state state)
{
- return h->state == STATE_READY;
+ return state == STATE_READY;
}
static int
@@ -81,25 +80,64 @@ is_processing_group (enum state_group group)
}
}
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_processing (struct nbd_handle *h)
+bool
+nbd_internal_is_state_processing (enum state state)
{
- enum state_group group = nbd_internal_state_group (h->state);
+ enum state_group group = nbd_internal_state_group (state);
return is_processing_group (group);
}
+bool
+nbd_internal_is_state_dead (enum state state)
+{
+ return state == STATE_DEAD;
+}
+
+bool
+nbd_internal_is_state_closed (enum state state)
+{
+ return state == STATE_CLOSED;
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_created (struct nbd_handle *h)
+{
+ return nbd_internal_is_state_created (h->state);
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
+{
+ return nbd_internal_is_state_connecting (h->state);
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_ready (struct nbd_handle *h)
+{
+ return nbd_internal_is_state_ready (h->state);
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_processing (struct nbd_handle *h)
+{
+ return nbd_internal_is_state_processing (h->state);
+}
+
/* NB: is_locked = false, may_set_error = false. */
int
nbd_unlocked_aio_is_dead (struct nbd_handle *h)
{
- return h->state == STATE_DEAD;
+ return nbd_internal_is_state_dead (h->state);
}
/* NB: is_locked = false, may_set_error = false. */
int
nbd_unlocked_aio_is_closed (struct nbd_handle *h)
{
- return h->state == STATE_CLOSED;
+ return nbd_internal_is_state_closed (h->state);
}
diff --git a/lib/rw.c b/lib/rw.c
index e3e0082..5fe3c64 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -201,7 +201,7 @@ nbd_internal_command_common (struct nbd_handle *h,
* be handled automatically on a future cycle around to READY.
*/
if (h->cmds_to_issue != NULL) {
- assert (nbd_unlocked_aio_is_processing (h));
+ assert (nbd_internal_is_state_processing (h->state));
prev_cmd = h->cmds_to_issue;
while (prev_cmd->next)
prev_cmd = prev_cmd->next;
@@ -209,7 +209,7 @@ nbd_internal_command_common (struct nbd_handle *h,
}
else {
h->cmds_to_issue = cmd;
- if (nbd_unlocked_aio_is_ready (h) &&
+ if (nbd_internal_is_state_ready (h->state) &&
nbd_internal_run (h, cmd_issue) == -1)
return -1;
}
--
2.21.0