Hi
On Thu, Dec 20, 2018 at 5:44 PM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:
>
> On Thu, Dec 20, 2018 at 05:37:40PM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel