On 7/13/23 21:29, Eric Blake wrote:
> Enhance the regression tests to prove that the completion callback is
> not reached if the aio call itself reports an error; only the .free
> callback is guaranteed.
>
> Also add some asserts to the library code that may aid future readers
> in seeing how we track transfer semantics of a callback.
>
> Goes hand in hand with the documentation cleanups made in the previous
> patch, but done separately for ease of backporting as nbd_aio_opt
> calls are not in as many releases.
>
> Reported-by: Tage Johansson <tage.j.li...@posteo.net>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> lib/opt.c | 2 +
> tests/Makefile.am | 5 +
> tests/errors-not-negotiating-aio.c | 170 +++++++++++++++++++++++++++++
> .gitignore | 1 +
> 4 files changed, 178 insertions(+)
> create mode 100644 tests/errors-not-negotiating-aio.c
Acked-by: Laszlo Ersek <ler...@redhat.com>
Laszlo
>
> diff --git a/lib/opt.c b/lib/opt.c
> index f58d5e19..1446eef3 100644
> --- a/lib/opt.c
> +++ b/lib/opt.c
> @@ -223,6 +223,7 @@ nbd_unlocked_opt_list (struct nbd_handle *h,
> nbd_list_callback *list)
> if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
> return -1;
>
> + assert (CALLBACK_IS_NULL (l));
> SET_CALLBACK_TO_NULL (*list);
> if (wait_for_option (h) == -1)
> return -1;
> @@ -269,6 +270,7 @@ opt_meta_context_queries (struct nbd_handle *h,
> if (aio_opt_meta_context_queries (h, opt, queries, &l, &c) == -1)
> return -1;
>
> + assert (CALLBACK_IS_NULL (l));
> SET_CALLBACK_TO_NULL (*context);
> if (wait_for_option (h) == -1)
> return -1;
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 3a93251e..6eddcb7a 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -171,6 +171,7 @@ check_PROGRAMS += \
> errors-connect-null \
> errors-connect-twice \
> errors-not-negotiating \
> + errors-not-negotiating-aio \
> errors-notify-not-blocked \
> errors-bad-cookie \
> errors-pread-structured \
> @@ -243,6 +244,7 @@ TESTS += \
> errors-connect-null \
> errors-connect-twice \
> errors-not-negotiating \
> + errors-not-negotiating-aio \
> errors-notify-not-blocked \
> errors-bad-cookie \
> errors-pread-structured \
> @@ -332,6 +334,9 @@ errors_connect_twice_LDADD = $(top_builddir)/lib/libnbd.la
> errors_not_negotiating_SOURCES = errors-not-negotiating.c
> errors_not_negotiating_LDADD = $(top_builddir)/lib/libnbd.la
>
> +errors_not_negotiating_aio_SOURCES = errors-not-negotiating-aio.c
> +errors_not_negotiating_aio_LDADD = $(top_builddir)/lib/libnbd.la
> +
> errors_notify_not_blocked_SOURCES = errors-notify-not-blocked.c
> errors_notify_not_blocked_LDADD = $(top_builddir)/lib/libnbd.la
>
> diff --git a/tests/errors-not-negotiating-aio.c
> b/tests/errors-not-negotiating-aio.c
> new file mode 100644
> index 00000000..b09cae82
> --- /dev/null
> +++ b/tests/errors-not-negotiating-aio.c
> @@ -0,0 +1,170 @@
> +/* NBD client library in userspace
> + * Copyright Red Hat
> + *
> + * 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
> + */
> +
> +/* Deliberately provoke some errors and check the error messages from
> + * nbd_get_error etc look reasonable.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <libnbd.h>
> +
> +static char *progname;
> +
> +static void
> +check (int experr, const char *prefix)
> +{
> + const char *msg = nbd_get_error ();
> + int errnum = nbd_get_errno ();
> +
> + fprintf (stderr, "error: \"%s\"\n", msg);
> + fprintf (stderr, "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);
> + }
> +}
> +
> +struct progress {
> + int id;
> + bool call_freed;
> + bool comp_freed;
> +};
> +
> +static int
> +list_call (void *user_data, const char *name, const char *description)
> +{
> + struct progress *p = user_data;
> +
> + fprintf (stderr, "%s: list callback should not be reached, iter %d\n",
> + progname, p->id);
> + exit (EXIT_FAILURE);
> +}
> +
> +static void
> +list_free (void *user_data)
> +{
> + struct progress *p = user_data;
> +
> + if (p->comp_freed) {
> + fprintf (stderr, "%s: list free called too late, iter %d\n",
> + progname, p->id);
> + exit (EXIT_FAILURE);
> + }
> + p->call_freed = true;
> +}
> +
> +static int
> +comp_call (void *user_data, int *error)
> +{
> + struct progress *p = user_data;
> +
> + fprintf (stderr, "%s: list callback should not be reached, iter %d\n",
> + progname, p->id);
> + exit (EXIT_FAILURE);
> +}
> +
> +static void
> +comp_free (void *user_data)
> +{
> + struct progress *p = user_data;
> +
> + if (!p->call_freed) {
> + fprintf (stderr, "%s: completion free called too early, iter %d\n",
> + progname, p->id);
> + exit (EXIT_FAILURE);
> + }
> + p->comp_freed = true;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> + struct nbd_handle *nbd;
> + const char *cmd[] = {
> + "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "1048576", NULL
> + };
> + struct progress p;
> + nbd_list_callback list_cb = { .callback = list_call,
> + .user_data = &p,
> + .free = list_free };
> + nbd_completion_callback comp_cb = { .callback = comp_call,
> + .user_data = &p,
> + .free = comp_free };
> +
> + progname = argv[0];
> +
> + nbd = nbd_create ();
> + if (nbd == NULL) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Too early to try nbd_aio_opt_*. */
> + p = (struct progress) { .id = 0 };
> + if (nbd_aio_opt_list (nbd, list_cb, comp_cb) != -1) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_opt_list did not reject attempt in wrong state\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> + check (ENOTCONN, "nbd_aio_opt_list: ");
> + if (!p.call_freed || !p.comp_freed) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_opt_list did not call .free callbacks\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Connect to the server without requesting negotiation mode. */
> + if (nbd_connect_command (nbd, (char **)cmd) == -1) {
> + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Too late to try nbd_aio_opt_*. */
> + p = (struct progress) { .id = 1 };
> + if (nbd_aio_opt_list (nbd, list_cb, comp_cb) != -1) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_opt_list did not reject attempt in wrong state\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> + check (EINVAL, "nbd_aio_opt_list: ");
> + if (!p.call_freed || !p.comp_freed) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_opt_list did not call .free callbacks\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> +
> + nbd_close (nbd);
> + exit (EXIT_SUCCESS);
> +}
> diff --git a/.gitignore b/.gitignore
> index efe3080a..b43e83c0 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -220,6 +220,7 @@ Makefile.in
> /tests/errors-name-too-long
> /tests/errors-not-connected
> /tests/errors-not-negotiating
> +/tests/errors-not-negotiating-aio
> /tests/errors-notify-not-blocked
> /tests/errors-poll-no-fd
> /tests/errors-pread-structured
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
