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

[Daniel P. Berrangé]

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 ?

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

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 :|