[Libguestfs] [PATCH libnbd 2/4] lib: Split nbd_aio_is_* functions into internal.

Richard W.M. Jones
2019-06-05T11:15:35Z

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