<div dir="ltr">Hi<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 17, 2015 at 3:35 PM, Marc-André Lureau <span dir="ltr"><<a href="mailto:mlureau@redhat.com" target="_blank">mlureau@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class=""><br> <br> ----- Original Message -----<br> > Hi<br> > On Thu, Sep 17, 2015 at 2:47 PM, Marc-André Lureau < <a href="mailto:mlureau@redhat.com">mlureau@redhat.com</a> ><br> > wrote:<br> ><br> ><br> > Hi<br> ><br> > ----- Original Message -----<br> ><br> > > How do you prevent from having multiple drive pointing to the spice folder?<br> > ><br> > ><br> > > Currently, I don't.<br> > ><br> > > Only way to connect multiple drives, is if you restart the service,<br> > > because the drives will disconnect, after restarting guest.<br> > ><br> > > I will add a disconnect function to the quit function in the service,<br> > > so that if uou disable, or restart teh service, the drive will be unmapped<br> > ><br> ><br> > Can't you enumerate existing mappings and only add one if it's necessary?<br> ><br> ><br> > Yes there seems to be API for that, so I can try to implement it.<br> ><br> > Still I'll make a disconnect function anyway, because it doesn't make sense<br> > to have a drive mapped, when the webdavd service is not even running<br> ><br> <br> </span>That could be annoying, I would rather not destroy user-defined mappings if possible.<br></blockquote><div><br></div><div>It won't touch user defined mappings, just the Spice client drive. I am trying to disconnect<br>using remote name, but it doesn't seem to be working, but I can remember what letter<br>I mapped to and disconnect that.<br><br></div><div>The big problem is that on Windows, if the drive is not connected, and you accidentally try to open it,<br>bad things WILL happen (explorer hangs for around 10 seconds or more). I think this is fixed in W10,<br>but I'm not entirely sure.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"><br> ><br> ><br> > > > ---<br> > > > Changes since RFC:<br> > > > - Calling WNetAddConnection2() blindly was slow and caause many threads<br> > > > to<br> > > > spawn.<br> > > > - Now only call it once, when it is sure, that it will connect.<br> > > > - Now connects to a * drive, instead of always Z:<br> > > > - Thread is now spawned and joined every time run_service() is called.<br> > > > ---<br> > > > Makefile.am | 4 +++<br> > > > spice/spice-webdavd.c | 76<br> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++<br> > > > 2 files changed, 80 insertions(+)<br> > > ><br> > > > diff --git a/Makefile.am b/Makefile.am<br> > > > index 485417b..df9a73e 100644<br> > > > --- a/Makefile.am<br> > > > +++ b/Makefile.am<br> > > > @@ -87,6 +87,10 @@ spice_webdavd_LDADD = \<br> > > > $(PIE_LDFLAGS) \<br> > > > $(NULL)<br> > > ><br> > > > +if OS_WIN32<br> > > > +spice_webdavd_LDADD += -lnetapi32 -lmpr<br> > > > +endif<br> > > > +<br> > > > deps.txt:<br> > > > $(AM_V_GEN)rpm -qa | grep $(host_os) | sort | unix2dos > $@<br> > > ><br> > > > diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c<br> > > > index f9c5cf1..3940f1c 100644<br> > > > --- a/spice/spice-webdavd.c<br> > > > +++ b/spice/spice-webdavd.c<br> > > > @@ -737,11 +737,81 @@ open_mux_path (const char *path)<br> > > > mux_queue = output_queue_new (G_OUTPUT_STREAM (mux_ostream));<br> > > > }<br> > > ><br> > > > +#ifdef G_OS_WIN32<br> > > > +static gchar<br> > > > +get_free_drive_letter(void)<br> > > > +{<br> > > > + const guint32 max_mask = 1 << 25;<br> > > > + guint32 drives;<br> > > > + guint32 mask;<br> > > > + gint i;<br> > > > +<br> > > > + drives = GetLogicalDrives ();<br> > > > +<br> > > > + for (i = 0; i < 26; i++)<br> > > > + {<br> > > > + mask = max_mask >> i;<br> > > > + if ((drives & mask) == 0)<br> > > > + return 'Z' - i;<br> > > > + }<br> > > > +<br> > > > + return 0;<br> > > > +}<br> > > > +<br> > > > +static gpointer<br> > > > +map_drive(gpointer user_data)<br> > > > +{<br> > > > + GCancellable * cancel_map = user_data;<br> > > > + gchar drive_letter;<br> > > > + gchar local_name[3];<br> > > > + gchar remote_name[] = " <a href="http://localhost:9843/" rel="noreferrer" target="_blank">http://localhost:9843/</a> ";<br> > > > + NETRESOURCE net_resource;<br> > > > + guint32 retval;<br> > > > + gint i;<br> > > > +<br> > > > + for (i = 0; i < 5; ++i)<br> > > > + {<br> > > > + if (g_cancellable_is_cancelled (cancel_map))<br> > > > + return NULL;<br> > > > + else<br> > > > + g_usleep (G_USEC_PER_SEC);<br> > > > + }<br> > ><br> > > It looks like this would fail if the guest is started without client<br> > > connected (or not sharing folder).<br> > ><br> > > I'm not sure I follow.<br> > > This is supposed to return NULL precisely, if the client is not connected,<br> > > or not sharing folder. Windows webdavd will loop and call run_service(),<br> > > until sharing is enabled, which will then NOT cancel this function,<br> > > and we map the drive.<br> ><br> > ah so the service keeps running and next time the client connects, then the<br> > thread will be started again?<br> ><br> > No, the thread starts only after the service is started for the first time.<br> > When user disconnects, the service<br> > currently doesn't notice on Windows (read_thread is still blocked), so when a<br> > different user connects,<br> <br> </div></div>Very often VM are started without a client connected.<br> <span class=""><br></span></blockquote><div><br></div><div>Of course, but that is not an issue. The client was looping and trying to connect even before my patches.<br><br></div><div>When the VM is started, it will spawn the thread, try to connect, and join it back after 5 seconds. Then it will loop and try again.<br></div><div>When client connects, and enables sharing, the next time the thread is spawned, it will connect and stop looping.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class=""> > the drive is still mapped, but when the new user enables sharing, the<br> > original drive will work, as it connects<br> > to the same remote name ( <a href="http://localhost:9843/" rel="noreferrer" target="_blank">http://localhost:9843/</a> ).<br> ><br> > This is still an issue, because if the new user doesn't enable sharing, the<br> > drive will still be there, but won't<br> > be accessible. But that is issue for another patch (and bug).<br> <br> </span>At least it would be nice to put this limitation in the commit message.<br></blockquote><div><br></div><div>Ok I will put it there.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class=""><br> > > Sorry I didn't mean what the thread was doing, but rather why we need a<br> > > thread for this task.<br> ><br> > Well I could use a g_task or g_idle, or something like that, but g_idle isn't<br> > cancellable, and g_task<br> <br> </span>g_idle is cancellable.</blockquote><div><br></div><div>Hmm, I couldn't find in the documentation. Usually the functions have some kind of GCancellable parameter.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class=""><br> > seemed too complicated for this. When I tried to do this in the main_thread<br> > (as it just has to be a<br> <br> </span>gtask is supposed to be simpler (although I don't use it myself ;)<br> <span class=""><br> > different thread then read_thread), I ran into race conditions between the<br> > read_thread and map_drive,<br> > especially, when using sleep between polling for cancellable.<br> > Spawning a thread for a few seconds just seemed to be less complicated.<br> <br> </span>It's hardly convincing me, threads are more racy by nature.<br></blockquote><div><br></div><div>That's true, but since I just need a few lines of code in the thread, and it's not<br>accessing anything externally (other then the cancel), the thread API is simpler for what I need.<br><br></div><div>With glib functions (g_task, g_idle, GSimplaAsyncResult), I need to supply stuff<br>I either don't need, or I don't know what they're for. I just need something, that will<br>call a function after 5 seconds, and can be cancelled.<br>That's 2-3 parameters (callback, cancel, maybe time).<br><br></div><div>Since I don't have that much experience with GLibs Async functions, I chose a thread.<br>For me it's simpler to implement and to test, it's fewer lines of code, and since the function<br>it runs is simple, I don't think that there is any advantage in using something else.<br>More so, if the other functions spawn a thread internally anyway.<br><br></div><div>I tried using a timer, and removing it before it fired, but sometimes it fired, even when it wasn't supposed to.<br></div><div>I could pass a GCancellable to the timeout instead, and let it fire, but then I would have multiple timeouts<br>firing at the same time, for no reason (because I would cancel the cancellable).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> 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.<br></blockquote><div><br><div>Well I'm not actually reading anything. And I think that read_thread starts only after<br>everything is prepared, because If I start the service with sharing enabled, everything works fine.<br></div>If the user is not connected/sharing is not enabled, the only thing my patches do, is that the new thread<br>waits for a few seconds, and gets cancelled, when the main loop ends (which will happen almost immediately).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> 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.<br> <br> Why not wrap WNetAddConnection2 in a GSimplaAsyncThread and run it once the client connection is established?<br></blockquote><div><br></div><div>Well that's the problem. I don't know when the client is connected, because there is no signal/callback,<br>anything that would notify me, that the client connected. The only way I know, that the client is connected,<br>is that the service stops looping, because it blocked in read_thread (this was happening before my patches also).<br><br></div><div>When it blocks, I have no way to call anything, that would map the drive, or notify something to map the drive.<br>That's why the only way was to start something before the read blocked, wait for a few seconds, and if nothing<br>told it otherwise, it would map the drive. If the read didn't block, it would somehow notify this something,<br>that the client is not actually connected, and not to map the drive.<br><br></div><div>I spent a few weeks trying to implement something using already defined signals, g_poll, even windows specific<br>polling/async reading, but nothing worked.<br></div><div><br><br></div><div>The bottom line is that even when it is usually better to use something more high level then a thread,<br>I think it's safe to use a thread here, and using more high level stuff wouldn't bring any advantage.<br>When I tried implementing something other then a thread, the only thing that brought was a headache,<br>because they refused to work at all.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class="im"><br> > As for why this has to be in a different thread, then read_thread, is because<br> > if I call map_drive before<br> > read actually blocks, it returns error, as if it's not connected. I think<br> > this is because when we try to map<br> > the drive, client sends a message to the guest, and the read has to be ready<br> > to receive it, which in turn<br> > lets the map_drive to actually map the drive.<br> ><br> > I tryed using every possible way I could think of, even windows specific<br> > polling, but I couldn't make<br> > anything work, except just plain spawning another thread.<br> ><br> > Lukas Venhoda<br> ><br> </span><div class=""><div class="h5">> _______________________________________________<br> > Spice-devel mailing list<br> > <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a><br> > <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a><br> ><br> </div></div></blockquote></div><br></div><div class="gmail_extra">Lukas Venhoda<br></div></div></div>