On Sun, Nov 7, 2021 at 3:58 PM Richard W.M. Jones <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
