[Spice-devel] [phodav PATCH 4/4] spice-webdavd: Automount shared folder on Windows

[Lukas Venhoda]

On Thu, Sep 17, 2015 at 4:54 PM, Marc-André Lureau <mlureau at redhat.com>
wrote:

>
>
> ----- Original Message -----
>
> > > > Sorry I didn't mean what the thread was doing, but rather why we
> need a
> > > > thread for this task.
> > >
> > > Well I could use a g_task or g_idle, or something like that, but g_idle
> > > isn't
> > > cancellable, and g_task
> >
> > g_idle is cancellable.
> >
> > Hmm, I couldn't find in the documentation. Usually the functions have
> some
> > kind of GCancellable parameter.
>
> You can do g_source_remove(), no need for cancellable
>

Ah, ok.


>
> > It's hardly convincing me, threads are more racy by nature.
> >
> > That's true, but since I just need a few lines of code in the thread, and
> > it's not
> > accessing anything externally (other then the cancel), the thread API is
> > simpler for what I need.
> >
> > With glib functions (g_task, g_idle, GSimplaAsyncResult), I need to
> supply
> > stuff
> > I either don't need, or I don't know what they're for. I just need
> something,
> > that will
> > call a function after 5 seconds, and can be cancelled.
> > That's 2-3 parameters (callback, cancel, maybe time).
> >
> > Since I don't have that much experience with GLibs Async functions, I
> chose a
> > thread.
> > For me it's simpler to implement and to test, it's fewer lines of code,
> and
> > since the function
> > it runs is simple, I don't think that there is any advantage in using
> > something else.
> > More so, if the other functions spawn a thread internally anyway.
> >
> > I tried using a timer, and removing it before it fired, but sometimes it
> > fired, even when it wasn't supposed to.
> > I could pass a GCancellable to the timeout instead, and let it fire, but
> then
> > I would have multiple timeouts
> > firing at the same time, for no reason (because I would cancel the
> > cancellable).
> >
>
> not clear to me, but ok
>
> >
> >
> > Trying to read from the driver when the started is wrong, at least it
> should
> > be after the virtio and the listening socket have been successfully
> opened.
> >
> > Well I'm not actually reading anything. And I think that read_thread
> starts
> > only after
> > everything is prepared, because If I start the service with sharing
> enabled,
> > everything works fine.
> > If the user is not connected/sharing is not enabled, the only thing my
> > patches do, is that the new thread
> > waits for a few seconds, and gets cancelled, when the main loop ends
> (which
> > will happen almost immediately).
>
> Yes, so you could start it after the service is actually started and the
> virtio port is opened, no? (repeating myself here)
>

Ah you mean socket_service? I thought you meant the windos service.
Yeah I can move it just above start_mux_read (mux_istream);


>
> >
> > I can imagine it may block because WNetAddConnection2 function call is
> sync
> > and will probe the service. But if it's blocking then it will not be
> able to
> > handle its communication work.
> >
> > Why not wrap WNetAddConnection2 in a GSimplaAsyncThread and run it once
> the
> > client connection is established?
> >
> > Well that's the problem. I don't know when the client is connected,
> because
> > there is no signal/callback,
> > anything that would notify me, that the client connected. The only way I
> > know, that the client is connected,
> > is that the service stops looping, because it blocked in read_thread
> (this
> > was happening before my patches also).
> >
>
> > When it blocks, I have no way to call anything, that would map the
> drive, or
> > notify something to map the drive.
>
> read_async is.. async! the thread is a helper, the mainloop is free for
> other events/task.
>

But read_async is not the issue, read_thread is.
g_input_stream_read_all() is not async, so it blocks when there are no
data. It only receives data, after:
1. client connects
2. enables sharing
3. maps the drive


>
> > That's why the only way was to start something before the read blocked,
> wait
> > for a few seconds, and if nothing
> > told it otherwise, it would map the drive. If the read didn't block, it
> would
> > somehow notify this something,
> > that the client is not actually connected, and not to map the drive.
> >
> > I spent a few weeks trying to implement something using already defined
> > signals, g_poll, even windows specific
> > polling/async reading, but nothing worked.
>
> Check if the mainloop is working properly using a timer, it should be
> fine. Then we can realize it's blocking when you actuall call
> WNetAddConnection2, and then you realize it's the reason you need a thread.
>

The mainloop should work fine, the only think that's blocked is the
read_thread
(talking about a function called read_thread here)

I can try something else using mainloop, but just so we understand each
other,
I still need another thread. It doesn't matter, if it's mainloop, or mine
thread,
but it HAS to be a different thread then the one where read_thread() is
working.
We agree on that right?


>
> >
> > The bottom line is that even when it is usually better to use something
> more
> > high level then a thread,
> > I think it's safe to use a thread here, and using more high level stuff
> > wouldn't bring any advantage.
>
> My bottom line is: don't use thread if you don't need them
>
> > When I tried implementing something other then a thread, the only thing
> that
> > brought was a headache,
> > because they refused to work at all.
>
> That's a good sign that there is something you didn't solve properly.
>

Yes, I'm aware of that. But since thread worked properly, I had no issue
with using it.


>
> >
> >
> >
> > > As for why this has to be in a different thread, then read_thread, is
> > > because
> > > if I call map_drive before
> > > read actually blocks, it returns error, as if it's not connected. I
> think
> > > this is because when we try to map
> > > the drive, client sends a message to the guest, and the read has to be
> > > ready
> > > to receive it, which in turn
> > > lets the map_drive to actually map the drive.
> > >
> > > I tryed using every possible way I could think of, even windows
> specific
> > > polling, but I couldn't make
> > > anything work, except just plain spawning another thread.
> > >
> > > Lukas Venhoda
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> >
> > Lukas Venhoda
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>

Lukas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150917/dcb32518/attachment-0001.html>