[Spice-devel] [PATCH spice] reds: don't unlink existing UNIX socket

[Marc-André Lureau]

Hi

On Thu, Dec 20, 2018 at 5:44 PM Daniel P. Berrangé <berrange at redhat.com> wrote:
>
> On Thu, Dec 20, 2018 at 05:37:40PM +0400, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >
> > Since "reds: add Unix socket support" (commit
> > 5365caeaae537f564d160936e60f71b2dc964ad1), the Spice server will
> > remove existing socket before binding.
> >
> > Although it may seem handy at first, this is a bad idea, as it may
> > create confusion (or worse).
>
> Can you elaborate on what the problem is ?

If you start 2 server instances with the same address, you are
overriding the previous instance. I don't think that's a good idea.

>
> Not deleting the UNIX socket will cause QEMU to fail to startup if
> the socket already exists on disk.

Yes, that's the point.

>
> This will hit every single time QEMU is started because nothing is
> deleting the socket on shutdown. Even if something did delete it on
> shutdown, that's not guaranteed to run on crash.
>
> unlinking immediately before bind() is the normal accepted solution
> to this problem.
>
> > Fortunately, passing the Unix path is done mostly by command line,
> > with qemu addr= Spice argument. Libvirt uses fd-passing socketpair
> > instead, so the impact should be fairly limited, but we should
> > probably warn the users in the release notes if we adopt this patch.
>
> IMHO this would be a significant step backwards.
>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > ---
> >  server/reds.c              | 1 -
> >  server/tests/test-listen.c | 1 +
> >  2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/server/reds.c b/server/reds.c
> > index cdbb94cb..8cd2bc86 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2527,7 +2527,6 @@ static int reds_init_socket(const char *addr, int portnr, int family)
> >
> >          local.sun_family = AF_UNIX;
> >          g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
> > -        unlink(local.sun_path);
> >          len = SUN_LEN(&local);
> >          if (bind(slisten, (struct sockaddr *)&local, len) == -1) {
> >              perror("bind");
> > diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
> > index 640e8f12..ec92bed0 100644
> > --- a/server/tests/test-listen.c
> > +++ b/server/tests/test-listen.c
> > @@ -330,6 +330,7 @@ static void test_connect_unix(void)
> >      SpiceServer *server = spice_server_new();
> >      spice_server_set_name(server, "SPICE listen test");
> >      spice_server_set_noauth(server);
> > +    unlink("test-listen.unix");
> >      spice_server_set_addr(server, "test-listen.unix", SPICE_ADDR_FLAG_UNIX_ONLY);
> >      result = spice_server_init(server, event_loop.core);
> >      g_assert_cmpint(result, ==, 0);
> > --
> > 2.20.1.2.gb21ebb671b
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau