Libguestfs - Jul 2019 - Re: [libnbd PATCH] tests: Enhance errors test

Let's check for a quite a few more errors.  Among other things, this
adds some coverage for a few things I've patched recently.
---

And these enhancements set me up for my next fix: making NBD_CMD_DISC
prevent future commands.

 tests/errors.c | 167 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 154 insertions(+), 13 deletions(-)

diff --git a/tests/errors.c b/tests/errors.c
index 99d5820..415c378 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -27,12 +27,40 @@

 #include <libnbd.h>

+#define MAXSIZE (65 * 1024 * 1024) /* Oversize on purpose */
+
+static char *progname;
+static char buf[MAXSIZE];
+
+static void
+check (int experr, const char *prefix)
+{
+  const char *msg = nbd_get_error ();
+  int errnum = nbd_get_errno ();
+
+  printf ("error: \"%s\"\n", msg);
+  printf ("errno: %d (%s)\n", errnum, strerror (errnum));
+  if (strncmp (msg, prefix, strlen (prefix)) != 0) {
+    fprintf (stderr, "%s: test failed: missing context prefix: %s\n",
+             progname, msg);
+    exit (EXIT_FAILURE);
+  }
+  if (errnum != experr) {
+    fprintf (stderr, "%s: test failed: "
+             "expected errno = %d (%s), but got %d\n",
+             progname, experr, strerror (experr), errnum);
+    exit (EXIT_FAILURE);
+  }
+}
+
 int
 main (int argc, char *argv[])
 {
   struct nbd_handle *nbd;
-  const char *msg;
-  int errnum;
+  const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent", "memory",
+                        "size=128m", NULL };
+
+  progname = argv[0];

   nbd = nbd_create ();
   if (nbd == NULL) {
@@ -47,23 +75,136 @@ main (int argc, char *argv[])
              argv[0]);
     exit (EXIT_FAILURE);
   }
-  msg = nbd_get_error ();
-  errnum = nbd_get_errno ();
-  printf ("error: \"%s\"\n", msg);
-  printf ("errno: %d (%s)\n", errnum, strerror (errnum));
-  if (strncmp (msg, "nbd_pread: ", strlen ("nbd_pread: "))
!= 0) {
-    fprintf (stderr, "%s: test failed: missing context prefix: %s\n",
-             argv[0], msg);
+  check (ENOTCONN, "nbd_pread: ");
+
+  /* Request a name that is too long. */
+  memset (buf, 'a', 4999);
+  buf[4999] = '\0';
+  if (nbd_set_export_name (nbd, buf) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_set_export_name did not reject large name\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (ENAMETOOLONG, "nbd_set_export_name: ");
+
+  /* Poll while there is no fd. */
+  if (nbd_aio_get_fd (nbd) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_aio_get_fd did not fail prior to connection\n",
+             argv[0]);
+  }
+  check (EINVAL, "nbd_aio_get_fd: ");
+  if (nbd_poll (nbd, 1000) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_poll did not fail prior to connection\n",
+             argv[0]);
+  }
+  check (EINVAL, "nbd_poll: ");
+
+  /* Connect to a working server, then try to connect again. */
+  if (nbd_connect_command (nbd, (char **) cmd) == -1) {
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_connect_command (nbd, (char **) cmd) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_connect_command did not reject repeat attempt\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_connect_command: ");
+
+  /* Try to notify that writes are ready when we aren't blocked on POLLOUT
*/
+  if (nbd_aio_notify_write (nbd) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_aio_notify_write in wrong state did not fail\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_aio_notify_write: ");
+
+  /* Check for status of a bogus handle */
+  if (nbd_aio_command_completed (nbd, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_aio_command_completed on bogus handle did not
fail\n",
+             argv[0]);
     exit (EXIT_FAILURE);
   }
-  if (errnum != ENOTCONN) {
+  check (EINVAL, "nbd_aio_command_completed: ");
+
+  /* Read from an invalid offset */
+  if (nbd_pread (nbd, NULL, 0, -1, 0) != -1) {
     fprintf (stderr, "%s: test failed: "
-             "expected errno = ENOTCONN, but got %d\n",
-             argv[0], errnum);
+             "nbd_pread did not fail with bogus offset\n",
+             argv[0]);
     exit (EXIT_FAILURE);
   }
+  check (EINVAL, "nbd_pread: ");

-  /* XXX Test some more stuff here. */
+  /* Use unknown command flags */
+  if (nbd_pread (nbd, NULL, 0, 0, -1) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_pread did not fail with bogus flags\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_pread: ");
+
+  /* Check that oversized requests are rejected */
+  if (nbd_pread (nbd, buf, MAXSIZE, 0, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_pread did not fail with oversize request\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (ERANGE, "nbd_pread: ");
+  if (nbd_aio_pwrite (nbd, buf, MAXSIZE, 0, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_aio_pwrite did not fail with oversize request\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (ERANGE, "nbd_aio_pwrite: ");
+
+  /* Queue up a write command so large that we block on POLLIN, then queue
+   * multiple disconnects. XXX The last one should fail.
+   */
+  if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, 0) == -1) {
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if ((nbd_aio_get_direction (nbd) & LIBNBD_AIO_DIRECTION_WRITE) == 0) {
+    fprintf (stderr, "%s: test failed: "
+             "expect to be blocked on write\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_disconnect (nbd, 0) == -1) {
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_disconnect (nbd, 0) == -1) { /* XXX */
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Flush the queue (whether this one fails is a race with how fast
+   * the server shuts down, so don't enforce status), then try to send
+   * another command while CLOSED/DEAD
+   */
+  if (nbd_shutdown (nbd) == -1) {
+    fprintf (stderr, "%s: ignoring %s\n", argv[0], nbd_get_error ());
+  }
+  else
+    fprintf (stderr, "%s: shutdown completed successfully\n",
argv[0]);
+  if (nbd_pread (nbd, NULL, 0, 0, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_pread did not fail on non-connected handle\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_pread: ");

   nbd_close (nbd);
   exit (EXIT_SUCCESS);
-- 
2.20.1

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Thu, Jun 27, 2019 at 10:18:30PM -0500, Eric Blake
wrote:
> +  /* Queue up a write command so large that we block on POLLIN, then queue
> +   * multiple disconnects. XXX The last one should fail.
> +   */
> +  if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, 0) == -1) {
> +    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +  if ((nbd_aio_get_direction (nbd) & LIBNBD_AIO_DIRECTION_WRITE) == 0)
{
> +    fprintf (stderr, "%s: test failed: "
> +             "expect to be blocked on write\n",
> +             argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
This test fails when run under valgrind.  An abbreviated log shows
what's happening:

libnbd: debug: nbd_aio_pwrite: event CmdIssue: READY -> ISSUE_COMMAND.START
libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.START ->
ISSUE_COMMAND.
SEND_REQUEST
libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_REQUEST ->
ISSUE_C
OMMAND.PREPARE_WRITE_PAYLOAD
libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD
-
> ISSUE_COMMAND.SEND_WRITE_PAYLOAD
libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_WRITE_PAYLOAD
-> I
SSUE_COMMAND.FINISH
libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.FINISH -> READY
/home/rjones/d/libnbd/tests/.libs/lt-errors: test failed: expect to be blocked
on write

It seems as if this is caused by valgrinded code running more slowly,
rather than an actual valgrind/memory error.

I wonder if we could remove the race using a custom nbdkit-sh-plugin
which would block on writes until (eg) a local trigger file was
touched?  Even that seems as if it would depend on the amount of data
that the kernel is able to buffer.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 6/30/19 12:54 PM, Richard W.M. Jones wrote:
> On Thu, Jun 27, 2019 at 10:18:30PM -0500, Eric Blake wrote:
>> +  /* Queue up a write command so large that we block on POLLIN, then
queue
>> +   * multiple disconnects. XXX The last one should fail.
>> +   */
>> +  if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, 0) == -1) {
>> +    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
>> +    exit (EXIT_FAILURE);
>> +  }
>> +  if ((nbd_aio_get_direction (nbd) & LIBNBD_AIO_DIRECTION_WRITE)
== 0) {
>> +    fprintf (stderr, "%s: test failed: "
>> +             "expect to be blocked on write\n",
>> +             argv[0]);
>> +    exit (EXIT_FAILURE);
>> +  }
> 
> This test fails when run under valgrind.  An abbreviated log shows
> what's happening:
> 
> libnbd: debug: nbd_aio_pwrite: event CmdIssue: READY ->
ISSUE_COMMAND.START
> libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.START ->
ISSUE_COMMAND.
> SEND_REQUEST
> libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_REQUEST ->
ISSUE_C
> OMMAND.PREPARE_WRITE_PAYLOAD
> libnbd: debug: nbd_aio_pwrite: transition:
ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD -
>> ISSUE_COMMAND.SEND_WRITE_PAYLOAD
> libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_WRITE_PAYLOAD
-> I
> SSUE_COMMAND.FINISH
> libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.FINISH -> READY
> /home/rjones/d/libnbd/tests/.libs/lt-errors: test failed: expect to be
blocked on write
> 
> It seems as if this is caused by valgrinded code running more slowly,
> rather than an actual valgrind/memory error.
Or even that valgrind's interception of send()/recv() performs buffering
differently than we get by default from the kernel.  I don't know if
running strace on valgrind is a sensible enough thing to do to see
syscall behavior?
> 
> I wonder if we could remove the race using a custom nbdkit-sh-plugin
> which would block on writes until (eg) a local trigger file was
> touched?  Even that seems as if it would depend on the amount of data
> that the kernel is able to buffer.
I don't know how to make an nbdkit plugin stop the code in nbdkit/server
from read()ing from the client (the plugin code doesn't get to run until
the core has learned that the client wants a command serviced).  But it
may be possible to tweak things to send back-to-back write requests,
where even if the first write request gets sent completely, the plugin
can delay responding to that first write and use --filter=noparallel to
prevent the second command from reaching nbdkit.  I'll play with that,
to see if I can reproduce the valgrind race, as well as work around it
with back-to-back write commands to increase the likelihood of actually
preventing nbdkit from consuming the second command.

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