[Spice-devel] [spice-gtk v1 1/2] channel-webdav: implement channel-reset
Victor Toso
victortoso at redhat.com
Wed Aug 2 09:35:32 UTC 2017
Hi,
Thanks for the quick review!
On Tue, Aug 01, 2017 at 09:37:04AM -0400, Marc-André Lureau wrote:
>
>
> ----- Original Message -----
> > From: Victor Toso <me at victortoso.com>
> >
> > The channel can be reset after disabling the shared-folder. If we had
> > pending read, we should cancel it using the GCancellable for the
> > demuxing code on vmcstream.c (c->cancellable) and per client operation
> > (client->cancellable) which is done on client_remove_unref() called by
> > g_hash_table_unref() in this patch.
> >
> > This bug resolves rhbz#1474074 for Linux guests, tested on Fedora 25.
> >
> > Reproducer:
> > 1. With remote-viewer, connect to a Fedora 25 (GNOME) with
> > spice-webavd running;
> > 2. In remote-viewer, enable shared-folder (File > Preferences);
> > 3. In the guest, open Nautilus, go to "Other Locations" and double
> > click at "Spice client folder" to mount that webdav folder;
> > 4. Wait for the folder to be mounted in the guest;
> > 5. In remote-viewer, disabled shared-folder;
> > 6. In the guest, try to access the mounted folder. It should fail and
> > the mount point will be removed;
> > 7. Repeat steps 2->3 and see the client crash.
> >
> > > Thread 1 "remote-viewer" received signal SIGSEGV, Segmentation fault.
> > > (gdb) bt full
> > > #0 in __memmove_avx_unaligned_erms () from /lib64/libc.so.6
> > > #1 in spice_channel_read_sasl (channel=0xca8130, data=0xfc2050, len=6) at
> > > spice-channel.c:1122
> > > #2 in spice_channel_read (channel=0xca8130, data=0xfc2050, length=6) at
> > > spice-channel.c:1149
> > > #3 in spice_channel_recv_msg (channel=0xca8130,
> > msg_handler=0x7ffff4e52e3c <spice_webdav_handle_msg>, data=0x0) at
> > spice-channel.c:2031
> > > #4 in spice_channel_iterate_read (channel=0xca8130) at
> > > spice-channel.c:2338
> > > #5 in spice_channel_iterate (channel=0xca8130) at spice-channel.c:2376
> > > #6 in spice_channel_coroutine (data=0xca8130) at spice-channel.c:2664
> > > #7 0x00007ffff4e92d49 in coroutine_trampoline (cc=0xca77e0) at
> > > coroutine_ucontext.c:63
> > > #8 0x00007ffff4e92a01 in continuation_trampoline (i0=13268960, i1=0) at
> > > continuation.c:55
> > > #9 0x00007fffefce7600 in ?? () from /lib64/libc.so.6
> >
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1474074
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> > src/channel-webdav.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > index 4a246b5..a4e2215 100644
> > --- a/src/channel-webdav.c
> > +++ b/src/channel-webdav.c
> > @@ -563,7 +563,29 @@ static void spice_webdav_channel_dispose(GObject
> > *object)
> >
> > static void spice_webdav_channel_up(SpiceChannel *channel)
> > {
> > + SpiceWebdavChannelPrivate *c = SPICE_WEBDAV_CHANNEL(channel)->priv;
> > +
> > CHANNEL_DEBUG(channel, "up");
> > +
> > + if (c->stream == NULL) {
> > + /* In case the channel has been reset */
> > + spice_webdav_channel_init(SPICE_WEBDAV_CHANNEL(channel));
> > + }
> > +}
> > +
> > +static void spice_webdav_channel_reset(SpiceChannel *channel, gboolean
> > migrating)
> > +{
> > + SpiceWebdavChannel *self = SPICE_WEBDAV_CHANNEL(channel);
> > + SpiceWebdavChannelPrivate *c = self->priv;
> > +
> > + c->demuxing = FALSE;
> > + g_cancellable_cancel(c->cancellable);
> > + g_clear_object(&c->cancellable);
> > + g_hash_table_unref(c->clients);
>
> hmm, it's only created at _init() time.
>
> I think it would be better to call port_set_opened(self, false) in
> spice_port_channel_reset(), this would make this code redundant, and
> it would be quite logical.
Hm, that did not work. The reason is that channel_reset() from
spice-channel seems to be called before spice_webdav_channel_reset() and
spice_port_channel_reset(). At the time we do port_set_opened(self,
false), the event does not reach port_event() from channel-webdav.c
Well, that happens only if I implement channel_reset() in
channel-webdav.c. If I move the two changes below to port_event() and
remove channel_reset(), it works. Do you know why?
> However, I am not clear about migration handling, if it was tested, it
> was a long time ago...
>
> We may want to keep some state if migrating=true..
Yes, I've tested on Windows 7 which is the one I expect issues ;)
With and without this patch, the mount point is not lost but I agree
that the change below should affect any ongoing operation and likely
will break the communication...
> > + g_clear_pointer(&c->queue, output_queue_free);
> > + g_clear_object(&c->stream);
>
> Same problem it's created only at _init() time
>
> nack for now, it needs further work
I'll be sending a new patch soon! I think the correct behavior is
implementing channel-reset in channel-webdav and handling correctly the
case migrating==true but I don't really think that's a hard requirement
for now.
Cheers,
toso
> > +
> > +
> > SPICE_CHANNEL_CLASS(spice_webdav_channel_parent_class)->channel_reset(channel,
> > migrating);
> > }
> >
> > static void spice_webdav_channel_class_init(SpiceWebdavChannelClass *klass)
> > @@ -575,6 +597,7 @@ static void
> > spice_webdav_channel_class_init(SpiceWebdavChannelClass *klass)
> > gobject_class->finalize = spice_webdav_channel_finalize;
> > channel_class->handle_msg = spice_webdav_handle_msg;
> > channel_class->channel_up = spice_webdav_channel_up;
> > + channel_class->channel_reset = spice_webdav_channel_reset;
> >
> > g_signal_override_class_handler("port-event",
> > SPICE_TYPE_WEBDAV_CHANNEL,
> > --
> > 2.13.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170802/3055f667/attachment.sig>
More information about the Spice-devel
mailing list