Richard W.M. Jones
2021-Nov-07 13:58 UTC
[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 at redhat.com> > --- > 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
Nir Soffer
2021-Nov-07 14:52 UTC
[Libguestfs] [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation
On Sun, Nov 7, 2021 at 3:58 PM Richard W.M. Jones <rjones at redhat.com> wrote:> > 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 at redhat.com> > > --- > > 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");Thanks, I'll add it.> > + 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.This is the case: $ tests/connect-systemd-socket-activation Connecting handle 0 with systemd socket activation Using URI: nbd+unix:///?socket=/tmp/libnbd9BNa21/sock Connecting handle 1 to URI> 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.Yes we don't document that socket activation uses unix socket, and using this may break in future versions. I think this the user should be in control here. For example: nbd_connect_systemd_socket_activation(h, args, LIBNBD_SA_UNIX) If the user does not care about the type of the socket: nbd_connect_systemd_socket_activation(h, args, LIBNBD_SA_AUTO)> Also this test needs to check for nbd_can_multi_conn >= 1 before > actually doing this (although nbdkit will always return it).Sure, although this is not an example code, just a test. We can cut corners to simplify the test.> > + 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 suspect this is caused by multiple bugs: - libnbd sends SIGTERM and wait - if the server does not terminate, it will block forever. - nbdkit does not terminate if there are active connections? (I did not investigate yet) So this is not an API issue, just rough edges in libnbd.> I support the idea of making this work, but I'm not sure that doing it > this way is a good API.All this can be done by the user outside of libnbd, but using socket activation in linbd makes this really easy to use.> 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 >