Libguestfs - Jun 2019 - [PATCH libnbd 0/4] lib: Atomically update h->state.

I need to think about this patch series a bit more, but it
does at least pass the tests.

Rich.

Simple code motion.
---
 lib/Makefile.am |   1 +
 lib/aio.c       |  78 -----------------------------------
 lib/is-state.c  | 105 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 78 deletions(-)

diff --git a/lib/Makefile.am b/lib/Makefile.am
index 312545e..72d2d0b 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -42,6 +42,7 @@ libnbd_la_SOURCES = \
 	flags.c \
 	handle.c \
 	internal.h \
+	is-state.c \
 	nbd-protocol.h \
 	poll.c \
 	protocol.c \
diff --git a/lib/aio.c b/lib/aio.c
index a129af2..38e0318 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -48,84 +48,6 @@ nbd_unlocked_aio_notify_write (struct nbd_handle *h)
   return nbd_internal_run (h, notify_write);
 }
 
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_created (struct nbd_handle *h)
-{
-  return h->state == STATE_START;
-}
-
-static int
-is_connecting_group (enum state_group group)
-{
-  switch (group) {
-  case GROUP_TOP:
-    return 0;
-  case GROUP_CONNECT:
-  case GROUP_CONNECT_TCP:
-  case GROUP_CONNECT_COMMAND:
-  case GROUP_MAGIC:
-  case GROUP_OLDSTYLE:
-  case GROUP_NEWSTYLE:
-    return 1;
-  default:
-    return is_connecting_group (nbd_internal_state_group_parent (group));
-  }
-}
-
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
-{
-  enum state_group group = nbd_internal_state_group (h->state);
-
-  return is_connecting_group (group);
-}
-
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_ready (struct nbd_handle *h)
-{
-  return h->state == STATE_READY;
-}
-
-static int
-is_processing_group (enum state_group group)
-{
-  switch (group) {
-  case GROUP_TOP:
-    return 0;
-  case GROUP_ISSUE_COMMAND:
-  case GROUP_REPLY:
-    return 1;
-  default:
-    return is_processing_group (nbd_internal_state_group_parent (group));
-  }
-}
-
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_processing (struct nbd_handle *h)
-{
-  enum state_group group = nbd_internal_state_group (h->state);
-
-  return is_processing_group (group);
-}
-
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_dead (struct nbd_handle *h)
-{
-  return h->state == STATE_DEAD;
-}
-
-/* NB: is_locked = false, may_set_error = false. */
-int
-nbd_unlocked_aio_is_closed (struct nbd_handle *h)
-{
-  return h->state == STATE_CLOSED;
-}
-
 int
 nbd_unlocked_aio_command_completed (struct nbd_handle *h,
                                     int64_t handle)
diff --git a/lib/is-state.c b/lib/is-state.c
new file mode 100644
index 0000000..5ed2ee9
--- /dev/null
+++ b/lib/is-state.c
@@ -0,0 +1,105 @@
+/* NBD client library in userspace
+ * Copyright (C) 2013-2019 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <errno.h>
+#include <inttypes.h>
+
+#include "internal.h"
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_created (struct nbd_handle *h)
+{
+  return h->state == STATE_START;
+}
+
+static int
+is_connecting_group (enum state_group group)
+{
+  switch (group) {
+  case GROUP_TOP:
+    return 0;
+  case GROUP_CONNECT:
+  case GROUP_CONNECT_TCP:
+  case GROUP_CONNECT_COMMAND:
+  case GROUP_MAGIC:
+  case GROUP_OLDSTYLE:
+  case GROUP_NEWSTYLE:
+    return 1;
+  default:
+    return is_connecting_group (nbd_internal_state_group_parent (group));
+  }
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
+{
+  enum state_group group = nbd_internal_state_group (h->state);
+
+  return is_connecting_group (group);
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_ready (struct nbd_handle *h)
+{
+  return h->state == STATE_READY;
+}
+
+static int
+is_processing_group (enum state_group group)
+{
+  switch (group) {
+  case GROUP_TOP:
+    return 0;
+  case GROUP_ISSUE_COMMAND:
+  case GROUP_REPLY:
+    return 1;
+  default:
+    return is_processing_group (nbd_internal_state_group_parent (group));
+  }
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_processing (struct nbd_handle *h)
+{
+  enum state_group group = nbd_internal_state_group (h->state);
+
+  return is_processing_group (group);
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_dead (struct nbd_handle *h)
+{
+  return h->state == STATE_DEAD;
+}
+
+/* NB: is_locked = false, may_set_error = false. */
+int
+nbd_unlocked_aio_is_closed (struct nbd_handle *h)
+{
+  return h->state == STATE_CLOSED;
+}
-- 
2.21.0

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

All accesses to the h->state field are redirected through these macros
(except when we create the handle).

This is simple refactoring.
---
 generator/generator | 18 +++++++++---------
 lib/connect.c       | 10 +++++-----
 lib/disconnect.c    |  8 ++++----
 lib/internal.h      |  3 +++
 lib/is-state.c      | 12 ++++++------
 lib/rw.c            |  4 ++--
 6 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/generator/generator b/generator/generator
index 8ed0a06..a3fbea9 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2415,11 +2415,11 @@ let generate_lib_states_c ()        pr "  enum
state next_state = %s;\n" state_enum;
       pr "\n";
       pr "  r = _enter_%s (h, &next_state, blocked);\n"
state_enum;
-      pr "  if (h->state != next_state) {\n";
+      pr "  if (get_state (h) != next_state) {\n";
       pr "    debug (h, \"transition: %%s -> %%s\",\n";
       pr "           \"%s\",\n" display_name;
       pr "           nbd_internal_state_short_string
(next_state));\n";
-      pr "    h->state = next_state;\n";
+      pr "    set_state (h, next_state);\n";
       pr "  }\n";
       pr "  return r;\n";
       pr "}\n";
@@ -2434,7 +2434,7 @@ let generate_lib_states_c ()    pr "  bool
blocked;\n";
   pr "\n";
   pr "  /* Validate and handle the external event. */\n";
-  pr "  switch (h->state)\n";
+  pr "  switch (get_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { display_name; state_enum; events } } as state) ->
@@ -2446,7 +2446,7 @@ let generate_lib_states_c ()            fun (e,
next_state) ->
             pr "    case %s:\n" (c_string_of_external_event e);
             if state != next_state then (
-              pr "      h->state = %s;\n"
next_state.parsed.state_enum;
+              pr "      set_state (h, %s);\n"
next_state.parsed.state_enum;
               pr "      debug (h, \"event %%s: %%s ->
%%s\",\n";
               pr "             \"%s\", \"%s\",
\"%s\");\n"
                  (string_of_external_event e)
@@ -2462,7 +2462,7 @@ let generate_lib_states_c ()    pr "  }\n";
   pr "\n";
   pr "  set_error (EINVAL, \"external event %%d is invalid in state
%%s\",\n";
-  pr "             ev, nbd_internal_state_short_string
(h->state));\n";
+  pr "             ev, nbd_internal_state_short_string (get_state
(h)));\n";
   pr "  return -1;\n";
   pr "\n";
   pr " ok:\n";
@@ -2470,7 +2470,7 @@ let generate_lib_states_c ()    pr "    blocked =
true;\n";
   pr "\n";
   pr "    /* Run a single step. */\n";
-  pr "    switch (h->state)\n";
+  pr "    switch (get_state (h))\n";
   pr "    {\n";
   List.iter (
     fun { parsed = { state_enum } } ->
@@ -2496,7 +2496,7 @@ let generate_lib_states_c ()    pr "{\n";
   pr "  int r = 0;\n";
   pr "\n";
-  pr "  switch (h->state)\n";
+  pr "  switch (get_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { state_enum; events } }) ->
@@ -2542,7 +2542,7 @@ let generate_lib_states_c ()    pr "const char
*\n";
   pr "nbd_unlocked_connection_state (struct nbd_handle *h)\n";
   pr "{\n";
-  pr "  switch (h->state)\n";
+  pr "  switch (get_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ comment; parsed = { display_name; state_enum } }) ->
@@ -2841,7 +2841,7 @@ 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";
+      pr "  enum state state = get_state (h);\n";
       let tests          List.map (
           function
diff --git a/lib/connect.c b/lib/connect.c
index 63d2234..b889f80 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -38,16 +38,16 @@
 static int
 error_unless_ready (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (h->state))
+  if (nbd_internal_is_state_ready (get_state (h)))
     return 0;
 
   /* Why did it fail? */
-  if (nbd_internal_is_state_closed (h->state)) {
+  if (nbd_internal_is_state_closed (get_state (h))) {
     set_error (0, "connection is closed");
     return -1;
   }
 
-  if (nbd_internal_is_state_dead (h->state))
+  if (nbd_internal_is_state_dead (get_state (h)))
     /* Don't set the error here, keep the error set when
      * the connection died.
      */
@@ -55,14 +55,14 @@ error_unless_ready (struct nbd_handle *h)
 
   /* Should probably never happen. */
   set_error (0, "connection in an unexpected state (%s)",
-             nbd_internal_state_short_string (h->state));
+             nbd_internal_state_short_string (get_state (h)));
   return -1;
 }
 
 static int
 wait_until_connected (struct nbd_handle *h)
 {
-  while (nbd_internal_is_state_connecting (h->state)) {
+  while (nbd_internal_is_state_connecting (get_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/disconnect.c b/lib/disconnect.c
index 355a1c9..423edaf 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -29,14 +29,14 @@
 int
 nbd_unlocked_shutdown (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (h->state) ||
-      nbd_internal_is_state_processing (h->state)) {
+  if (nbd_internal_is_state_ready (get_state (h)) ||
+      nbd_internal_is_state_processing (get_state (h))) {
     if (nbd_unlocked_aio_disconnect (h, 0) == -1)
       return -1;
   }
 
-  while (!nbd_internal_is_state_closed (h->state) &&
-         !nbd_internal_is_state_dead (h->state)) {
+  while (!nbd_internal_is_state_closed (get_state (h)) &&
+         !nbd_internal_is_state_dead (get_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/internal.h b/lib/internal.h
index 691a1eb..7290247 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -291,6 +291,9 @@ extern const char *nbd_internal_state_short_string (enum
state state);
 extern enum state_group nbd_internal_state_group (enum state state);
 extern enum state_group nbd_internal_state_group_parent (enum state_group
group);
 
+#define set_state(h,next_state) ((h)->state) = (next_state)
+#define get_state(h) ((h)->state)
+
 /* utils.c */
 extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
 extern size_t nbd_internal_string_list_length (char **argv);
diff --git a/lib/is-state.c b/lib/is-state.c
index 55d103b..51a2d47 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -104,40 +104,40 @@ nbd_internal_is_state_closed (enum state state)
 int
 nbd_unlocked_aio_is_created (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_created (h->state);
+  return nbd_internal_is_state_created (get_state (h));
 }
 
 /* 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);
+  return nbd_internal_is_state_connecting (get_state (h));
 }
 
 /* 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);
+  return nbd_internal_is_state_ready (get_state (h));
 }
 
 /* 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);
+  return nbd_internal_is_state_processing (get_state (h));
 }
 
 /* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_dead (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_dead (h->state);
+  return nbd_internal_is_state_dead (get_state (h));
 }
 
 /* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_closed (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_closed (h->state);
+  return nbd_internal_is_state_closed (get_state (h));
 }
diff --git a/lib/rw.c b/lib/rw.c
index 5fe3c64..b38d95b 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_internal_is_state_processing (h->state));
+    assert (nbd_internal_is_state_processing (get_state (h)));
     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_internal_is_state_ready (h->state) &&
+    if (nbd_internal_is_state_ready (get_state (h)) &&
         nbd_internal_run (h, cmd_issue) == -1)
       return -1;
   }
-- 
2.21.0

Split h->state into:

 - h->state = the state on entry to the locked region

 - h->next_state = the current state and what the "publicly
visible"
   state will become when we leave the locked region

Some calls to get_state become calls to get_next_state depending on
which of these they are trying to read.  Calls to set_state become
set_next_state because that is what gets updated.

When we leave the locked region we update h->state.

The purpose of this patch is to make it easier to reason about the
state in lockless code.
---
 generator/generator | 23 +++++++++++++----------
 lib/connect.c       | 10 +++++-----
 lib/disconnect.c    |  8 ++++----
 lib/handle.c        |  2 +-
 lib/internal.h      | 15 +++++++++++++--
 lib/rw.c            |  4 ++--
 6 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/generator/generator b/generator/generator
index a3fbea9..d198007 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2415,11 +2415,11 @@ let generate_lib_states_c ()        pr "  enum
state next_state = %s;\n" state_enum;
       pr "\n";
       pr "  r = _enter_%s (h, &next_state, blocked);\n"
state_enum;
-      pr "  if (get_state (h) != next_state) {\n";
+      pr "  if (get_next_state (h) != next_state) {\n";
       pr "    debug (h, \"transition: %%s -> %%s\",\n";
       pr "           \"%s\",\n" display_name;
       pr "           nbd_internal_state_short_string
(next_state));\n";
-      pr "    set_state (h, next_state);\n";
+      pr "    set_next_state (h, next_state);\n";
       pr "  }\n";
       pr "  return r;\n";
       pr "}\n";
@@ -2434,7 +2434,7 @@ let generate_lib_states_c ()    pr "  bool
blocked;\n";
   pr "\n";
   pr "  /* Validate and handle the external event. */\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { display_name; state_enum; events } } as state) ->
@@ -2446,7 +2446,7 @@ let generate_lib_states_c ()            fun (e,
next_state) ->
             pr "    case %s:\n" (c_string_of_external_event e);
             if state != next_state then (
-              pr "      set_state (h, %s);\n"
next_state.parsed.state_enum;
+              pr "      set_next_state (h, %s);\n"
next_state.parsed.state_enum;
               pr "      debug (h, \"event %%s: %%s ->
%%s\",\n";
               pr "             \"%s\", \"%s\",
\"%s\");\n"
                  (string_of_external_event e)
@@ -2462,7 +2462,7 @@ let generate_lib_states_c ()    pr "  }\n";
   pr "\n";
   pr "  set_error (EINVAL, \"external event %%d is invalid in state
%%s\",\n";
-  pr "             ev, nbd_internal_state_short_string (get_state
(h)));\n";
+  pr "             ev, nbd_internal_state_short_string (get_next_state
(h)));\n";
   pr "  return -1;\n";
   pr "\n";
   pr " ok:\n";
@@ -2470,7 +2470,7 @@ let generate_lib_states_c ()    pr "    blocked =
true;\n";
   pr "\n";
   pr "    /* Run a single step. */\n";
-  pr "    switch (get_state (h))\n";
+  pr "    switch (get_next_state (h))\n";
   pr "    {\n";
   List.iter (
     fun { parsed = { state_enum } } ->
@@ -2496,7 +2496,7 @@ let generate_lib_states_c ()    pr "{\n";
   pr "  int r = 0;\n";
   pr "\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { state_enum; events } }) ->
@@ -2542,7 +2542,7 @@ let generate_lib_states_c ()    pr "const char
*\n";
   pr "nbd_unlocked_connection_state (struct nbd_handle *h)\n";
   pr "{\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ comment; parsed = { display_name; state_enum } }) ->
@@ -2866,8 +2866,11 @@ let generate_lib_api_c ()      let argnames =
List.flatten (List.map name_of_arg args) in
     List.iter (pr ", %s") argnames;
     pr ");\n";
-    if is_locked then
-      pr "  pthread_mutex_unlock (&h->lock);\n";
+    if is_locked then (
+      pr "  if (h->state != h->next_state)\n";
+      pr "    h->state = h->next_state;\n";
+      pr "  pthread_mutex_unlock (&h->lock);\n"
+    );
     pr "  return ret;\n";
     pr "}\n";
     pr "\n";
diff --git a/lib/connect.c b/lib/connect.c
index b889f80..4e3141f 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -38,16 +38,16 @@
 static int
 error_unless_ready (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (get_state (h)))
+  if (nbd_internal_is_state_ready (get_next_state (h)))
     return 0;
 
   /* Why did it fail? */
-  if (nbd_internal_is_state_closed (get_state (h))) {
+  if (nbd_internal_is_state_closed (get_next_state (h))) {
     set_error (0, "connection is closed");
     return -1;
   }
 
-  if (nbd_internal_is_state_dead (get_state (h)))
+  if (nbd_internal_is_state_dead (get_next_state (h)))
     /* Don't set the error here, keep the error set when
      * the connection died.
      */
@@ -55,14 +55,14 @@ error_unless_ready (struct nbd_handle *h)
 
   /* Should probably never happen. */
   set_error (0, "connection in an unexpected state (%s)",
-             nbd_internal_state_short_string (get_state (h)));
+             nbd_internal_state_short_string (get_next_state (h)));
   return -1;
 }
 
 static int
 wait_until_connected (struct nbd_handle *h)
 {
-  while (nbd_internal_is_state_connecting (get_state (h))) {
+  while (nbd_internal_is_state_connecting (get_next_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/disconnect.c b/lib/disconnect.c
index 423edaf..95e9a37 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -29,14 +29,14 @@
 int
 nbd_unlocked_shutdown (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (get_state (h)) ||
-      nbd_internal_is_state_processing (get_state (h))) {
+  if (nbd_internal_is_state_ready (get_next_state (h)) ||
+      nbd_internal_is_state_processing (get_next_state (h))) {
     if (nbd_unlocked_aio_disconnect (h, 0) == -1)
       return -1;
   }
 
-  while (!nbd_internal_is_state_closed (get_state (h)) &&
-         !nbd_internal_is_state_dead (get_state (h))) {
+  while (!nbd_internal_is_state_closed (get_next_state (h)) &&
+         !nbd_internal_is_state_dead (get_next_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/handle.c b/lib/handle.c
index cc311ba..8bf5280 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -57,7 +57,7 @@ nbd_create (void)
   s = getenv ("LIBNBD_DEBUG");
   h->debug = s && strcmp (s, "1") == 0;
 
-  h->state = STATE_START;
+  h->state = h->next_state = STATE_START;
   h->pid = -1;
 
   h->export_name = strdup ("");
diff --git a/lib/internal.h b/lib/internal.h
index 7290247..1bef1c2 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -80,7 +80,17 @@ struct nbd_handle {
   /* Linked list of close callbacks. */
   struct close_callback *close_callbacks;
 
-  _Atomic enum state state;     /* State machine. */
+  /* State machine.
+   *
+   * The actual current state is ‘next_state’.  ‘state’ is updated
+   * before we release the lock.
+   *
+   * Note don't access these fields directly, use the SET_NEXT_STATE
+   * macro in generator/states* code, or the set_next_state,
+   * get_next_state and get_state macros in regular code.
+   */
+  _Atomic enum state state;
+  enum state next_state;
 
   bool structured_replies;      /* If we negotiated NBD_OPT_STRUCTURED_REPLY */
 
@@ -291,7 +301,8 @@ extern const char *nbd_internal_state_short_string (enum
state state);
 extern enum state_group nbd_internal_state_group (enum state state);
 extern enum state_group nbd_internal_state_group_parent (enum state_group
group);
 
-#define set_state(h,next_state) ((h)->state) = (next_state)
+#define set_next_state(h,_next_state) ((h)->next_state) = (_next_state)
+#define get_next_state(h) ((h)->next_state)
 #define get_state(h) ((h)->state)
 
 /* utils.c */
diff --git a/lib/rw.c b/lib/rw.c
index b38d95b..ad9c8a0 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_internal_is_state_processing (get_state (h)));
+    assert (nbd_internal_is_state_processing (get_next_state (h)));
     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_internal_is_state_ready (get_state (h)) &&
+    if (nbd_internal_is_state_ready (get_next_state (h)) &&
         nbd_internal_run (h, cmd_issue) == -1)
       return -1;
   }
-- 
2.21.0

On Wed, Jun 05, 2019 at 12:15:37PM +0100, Richard W.M. Jones
wrote:
> -#define set_state(h,next_state) ((h)->state) = (next_state)
> +#define set_next_state(h,_next_state) ((h)->next_state) = (_next_state)
> +#define get_next_state(h) ((h)->next_state)
>  #define get_state(h) ((h)->state)
So I wonder if it's better to rename get_state as get_last_state or
get_visible_state?  And/or rename get_next_state/set_next_state to
get_state/set_state?

Ideas welcome to make the code clearer.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 6/5/19 6:15 AM, Richard W.M. Jones wrote:
> Simple code motion.
> ---
>  lib/Makefile.am |   1 +
>  lib/aio.c       |  78 -----------------------------------
>  lib/is-state.c  | 105 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+), 78 deletions(-)
> 
> +++ b/lib/is-state.c
> @@ -0,0 +1,105 @@
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <errno.h>
> +#include <inttypes.h>
Do you really need all of these?  At least errno.h looks spurious after
a quick glance.

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 6/5/19 6:15 AM, Richard W.M. Jones wrote:
> 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.
> ---
> @@ -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)"
Yes, this fixes the race: this code is executed outside the lock, so it
must read h->state exactly once before using it in multiple spots.
> +++ 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))
error_unless_ready() is called while lock is held, so multiple reads of
h->state are fine here.
> +++ 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)) {
Likewise safe for multiple h->state reads.

> +/* 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);
> +}
While we've fixed our internal uses, external callers may still have a
race when calling multiple nbd_aio_is_* functions in a row, since the
state can change between those functions.  Is that a problem?  Are there
any common state combinations (such as is_ready||is_processing) that
warrant their own API entry points so as to be race-free?

At any rate, this is better than what we've had, and does fix a real
race (even if it doesn't solve everything yet), so ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 6/5/19 6:15 AM, Richard W.M. Jones wrote:
> All accesses to the h->state field are redirected through these macros
> (except when we create the handle).
> 
> This is simple refactoring.
> ---
>  generator/generator | 18 +++++++++---------
>  lib/connect.c       | 10 +++++-----
>  lib/disconnect.c    |  8 ++++----
>  lib/internal.h      |  3 +++
>  lib/is-state.c      | 12 ++++++------
>  lib/rw.c            |  4 ++--
>  6 files changed, 29 insertions(+), 26 deletions(-)
> 
ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 6/5/19 6:15 AM, Richard W.M. Jones wrote:
> Split h->state into:
> 
>  - h->state = the state on entry to the locked region
> 
>  - h->next_state = the current state and what the "publicly
visible"
>    state will become when we leave the locked region
The rest of this thread discusses potential other names, such as
h->public_state for the state visible to unlocked code and h->state for
internal while still locked; I trust that once you pick a naming scheme
you like, you can redo this patch accordingly. I'll go ahead and review
the changes as spelled here to at least see if I can spot any problems.
> 
> Some calls to get_state become calls to get_next_state depending on
> which of these they are trying to read.  Calls to set_state become
> set_next_state because that is what gets updated.
> 
> When we leave the locked region we update h->state.
> 
> The purpose of this patch is to make it easier to reason about the
> state in lockless code.
Lockless code (including nbd_aio_get_direction) can no longer see
intermediate states that did not block, which is a lot nicer for
deciding whether we will accidentally let the user's poll() loop block
forever if it didn't see that we were blocked on POLLIN or POLLOUT.

There's still always a TOCTTOU race (anywhere the user reads the public
state while another thread holds the lock, their future actions based on
the observed state may be unexpected by the time they actually grab the
lock because the other thread changed state), as well as the issue I
mentioned in on 3/4 about any consecutive user calls to nbd_aio_is_*
having a race where the two calls can be done on different public states.

I'm still thinking that for any functions that grab the lock but want to
do gating on state MUST save the public state check until after grabbing
the lock, not beforehand, to avoid the TOCTTOU race.
> ---
>  generator/generator | 23 +++++++++++++----------
>  lib/connect.c       | 10 +++++-----
>  lib/disconnect.c    |  8 ++++----
>  lib/handle.c        |  2 +-
>  lib/internal.h      | 15 +++++++++++++--
>  lib/rw.c            |  4 ++--
>  6 files changed, 38 insertions(+), 24 deletions(-)
> 
> @@ -2462,7 +2462,7 @@ let generate_lib_states_c () >    pr " 
}\n";
>    pr "\n";
>    pr "  set_error (EINVAL, \"external event %%d is invalid in
state %%s\",\n";
> -  pr "             ev, nbd_internal_state_short_string (get_state
(h)));\n";
> +  pr "             ev, nbd_internal_state_short_string
(get_next_state (h)));\n";
I'm also wondering if we should treat external events more like
interrupts, where calling nbd_aio_notify_read() in an incorrect state
succeeds by setting a flag that is not cleared until we later reach a
state that can actually care about the flag, rather than by rejecting
the call out-right for being in the wrong state.  (That is, states where
we are NOT expecting a notify_read() are the same as setting an IRQ mask
that we don't want to be interrupted by a a read notification yet, but
we don't want to lose the interrupt when we later clear the mask)

In fact, CmdIssue is somewhat already like this - the code determines if
the state machine is unmasked (in the READY state) to kick it off
immediately; or is masked (in a processing state) to queue things but
leave the interrupt set (the next time we enter READY, we unmask,
observe the queue is non-empty, and so fire off the next command).

Another thought - if we ever want to allow the user the ability to send
custom NBD_OPT_ commands during handshake, or even to emulate 'qemu-nbd
--list' where the client can make queries to see what the server
supports before finally settling on whether to run NBD_OPT_GO or
NBD_OPT_ABORT, we'll need to add an external event after nbd_aio_connect
but before nbd_aio_is_connected for doing those additional handshake
steps. It's easier to think about adding a mandatory nbd_aio_go() called
in between nbd_aio_connect*() and the first nbd_aio_pread now, before we
have to start worrying about back-compat issues to all existing AIO clients.

> @@ -2866,8 +2866,11 @@ let generate_lib_api_c () 
As said above, I think you're still missing a change at the beginning of
the wrapper that grabs the lock to do the state check after the lock.
>      let argnames = List.flatten (List.map name_of_arg args) in
>      List.iter (pr ", %s") argnames;
>      pr ");\n";
> -    if is_locked then
> -      pr "  pthread_mutex_unlock (&h->lock);\n";
> +    if is_locked then (
> +      pr "  if (h->state != h->next_state)\n";
> +      pr "    h->state = h->next_state;\n";
> +      pr "  pthread_mutex_unlock (&h->lock);\n"
> +    );
>      pr "  return ret;\n";
>      pr "}\n";
>      pr "\n";
> diff --git a/lib/connect.c b/lib/connect.c
> index b889f80..4e3141f 100644
> --- a/lib/connect.c
> +++ b/lib/internal.h
> @@ -80,7 +80,17 @@ struct nbd_handle {
>    /* Linked list of close callbacks. */
>    struct close_callback *close_callbacks;
>  
> -  _Atomic enum state state;     /* State machine. */
> +  /* State machine.
> +   *
> +   * The actual current state is ‘next_state’.  ‘state’ is updated
> +   * before we release the lock.
> +   *
> +   * Note don't access these fields directly, use the SET_NEXT_STATE
> +   * macro in generator/states* code, or the set_next_state,
> +   * get_next_state and get_state macros in regular code.
> +   */
> +  _Atomic enum state state;
> +  enum state next_state;
>  
Otherwise the patch looks like you caught the right places.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org