Re: [Libguestfs] [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation

On Sun, Nov 07, 2021 at 03:27:46PM +0200, Nir Soffer wrote: > Test connecting using systemd socket activation, and using the handle > URI to connect additional handles. > > The tests reveals a deadlock when closing the first handle owning the > nbdkit server. Closing the first handle last avoids this issue. > > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; > --- > tests/Makefile.am | 5 ++ > tests/connect-systemd-socket-activation.c | 83 +++++++++++++++++++++++ > 2 files changed, 88 insertions(+) > create mode 100644 tests/connect-systemd-socket-activation.c > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 7f00f6f..1451fb8 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -192,6 +192,7 @@ check_PROGRAMS += \ > opt-list \ > opt-info \ > opt-list-meta \ > + connect-systemd-socket-activation \ > connect-unix \ > connect-tcp \ > connect-tcp6 \ > @@ -235,6 +236,7 @@ TESTS += \ > opt-list \ > opt-info \ > opt-list-meta \ > + connect-systemd-socket-activation \ > connect-unix \ > connect-tcp \ > connect-tcp6 \ > @@ -424,6 +426,9 @@ opt_info_LDADD = $(top_builddir)/lib/libnbd.la > opt_list_meta_SOURCES = opt-list-meta.c > opt_list_meta_LDADD = $(top_builddir)/lib/libnbd.la > > +connect_systemd_socket_activation_SOURCES = connect-systemd-socket-activation.c > +connect_systemd_socket_activation_LDADD = $(top_builddir)/lib/libnbd.la > + > connect_unix_SOURCES = connect-unix.c > connect_unix_LDADD = $(top_builddir)/lib/libnbd.la > > diff --git a/tests/connect-systemd-socket-activation.c b/tests/connect-systemd-socket-activation.c > new file mode 100644 > index 0000000..d64319a > --- /dev/null > +++ b/tests/connect-systemd-socket-activation.c > @@ -0,0 +1,83 @@ > +/* NBD client library in userspace > + * Copyright (C) 2021 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 > + */ > + > +/* Test connecting using systemd socket activation. */ > + > +#include <config.h> > + > +#include <stdio.h> > +#include <stdlib.h> > + > +#include <libnbd.h> > + > +#define CONNECTIONS 4 > + > +int > +main (int argc, char *argv[]) > +{ > + char *args[] = {"nbdkit", "-f", "memory", "size=1m", NULL}; > + struct nbd_handle *nbd[CONNECTIONS] = {0}; > + char *uri = NULL; > + int result = EXIT_FAILURE; The test needs to skip if nbdkit and/or the memory plugin is not available. This can be done using: #include "requires.h" ... requires ("nbdkit --version"); requires ("nbdkit memory --version");

> + printf("Connecting handle 0 with systemd socket actication\n"); > + > + nbd[0] = nbd_create (); > + if (nbd[0] == NULL) > + goto out; > + > + if (nbd_connect_systemd_socket_activation (nbd[0], args) == -1) > + goto out; > + > + uri = nbd_get_uri (nbd[0]); > + if (uri == NULL) > + goto out; > + > + printf("Using URI: %s\n", uri); I'm not sure what nbd_get_uri should show here - possibly it *ought* to return NULL (since there's no URI that should connect back to a server launched with systemd-socket-activation). But later ... > + for (int i = 1; i < CONNECTIONS; i++) { > + printf ("Connecting handle %d to URI\n", i); > + > + nbd[i] = nbd_create (); > + if (nbd[i] == NULL) > + goto out; > + > + if (nbd_connect_uri (nbd[i], uri) == -1) > + goto out; > + } Now I'm intrigued. I'm guessing the URI exposes the internal socket we create (/tmp/libnbdXXXXXX/sock) for SSA.

Which sounds like an "interesting" corner of the API. It also prevents us from changing this in future -- eg. to use socketpair or a socket in the anonymous space.

Also this test needs to check for nbd_can_multi_conn >= 1 before actually doing this (although nbdkit will always return it).

> + printf ("All handles connected\n"); > + result = EXIT_SUCCESS; > + > +out: > + if (result == EXIT_FAILURE) > + fprintf (stderr, "%s\n", nbd_get_error ()); > + > + free (uri); > + > + /* Closing the first connection deadlocks, so we close the additional > + * connections first. */ And this seems like a bear-trap.

I support the idea of making this work, but I'm not sure that doing it this way is a good API.

Rich. > + for (int i = CONNECTIONS - 1; i >= 0; i--) { > + if (nbd[i]) { > + printf ("Closing handle %d\n", i); > + nbd_close (nbd[i]); > + } > + } > + > + exit (result); > +} > -- > 2.31.1 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v