Hi
On Thu, Feb 25, 2016 at 4:21 PM, Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:
> Hi
>
> On Thu, Feb 25, 2016 at 3:19 PM, Lukas Venhoda <lvenh...@redhat.com>
> wrote:
> > +static void
> > +map_drive(GTask *task,
> > + gpointer source_object,
> > + gpointer task_data,
> > + GCancellable *cancellable)
> > +{
> > + static int loop_time = G_USEC_PER_SEC/10;
> > + GCancellable * cancel_map = task_data;
> > + gchar local_name[3];
> > + gchar remote_name[] = "http://localhost:9843/";;
> > + NETRESOURCE net_resource;
> > + guint32 retval;
> > + gint i;
> > +
> > + for (i = 0; i < 5; ++i)
> > + {
> > + if (g_cancellable_is_cancelled (cancel_map))
> > + return;
> > + else
> > + g_usleep (loop_time);
> > + }
> > +
>
> I am not sure I see the point of waiting 0.5s here. Add a comment? (a
> better way would be to use g_poll with the pollfd cancellable can
> creats: no need to loop)
>
>
I tryed this using g_io_channel_win32_make_pollfd(), but the way the FD
(well actually file handle) is created on windows, it doesn't support
polling.
It might require a more in depth rewrite of the windows part of webdavd to
make it work.
> > + if ((drive_letter = get_free_drive_letter ()) == 0)
> > + g_error ("all drive letters already assigned.");
>
> This will abort(), I think it's better to have simply a critical. The
> webdav folder can be accessed through other means (browser etc)
>
Will change to critical
>
> > + local_name[0] = drive_letter;
> > + local_name[1] = ':';
> > + local_name[2] = 0;
>
> This is not strictly equivalent to "net use *", it will be racy. It
> probably needs to retry if the localname is already used.
>
That's why there's get_free_drive_letter function().
The localname will either be usable, or the driver cannot be mapped.
Do you know equivalent function to net use? I could find one.
>
> > + net_resource.dwType = RESOURCETYPE_DISK;
> > + net_resource.lpLocalName = local_name;
> > + net_resource.lpRemoteName = remote_name;
> > + net_resource.lpProvider = NULL;
> > +
> > + retval = WNetAddConnection2 (&net_resource, NULL, NULL,
> CONNECT_TEMPORARY);
> > +
> > + if (retval == NO_ERROR)
> > + g_debug ("map_drive ok");
> > + else if (retval == ERROR_ALREADY_ASSIGNED)
> > + g_debug ("map_drive already asigned");
>
> here, goto getfreedriver?
>
If someone maps the drive before I can?
Well a simple do while loop can't hurt here.
>
> > + else
> > + g_error ("map_drive error %d", retval);
>
> Please avoid g_error for non-fatal issues.
>
Will change to critical
>
> The map-driver.bat file uses a nifty hack to name the folder "Spice
> client" (Spice folder would make more sense). It would be really nice
> to name the folder here too.
>
>
Yes, I forgot about that, because I already have the reg hack in my
registry.
I can try to reproduce it programatically.
> > +
> > + return;
> > +}
> > +#endif
> > +
> > static void
> > run_service (void)
> > {
> > g_debug ("Run service");
> >
> > +#ifdef G_OS_WIN32
> > + GCancellable * cancel_map = g_cancellable_new ();
> > + GTask * map_drive_task = g_task_new (NULL, NULL, NULL, NULL);
>
> no unref? you probably should g_task_return() somewhere too.
>
If I don't set the GTask to return-on-cancel, I don't need to call
g_task_return(),
because the callback is empty.
>From the GTask doc:
Other than in that case, *task* will be completed when the GTaskThreadFunc
<https://developer.gnome.org/gio/stable/GTask.html#GTaskThreadFunc> returns,
not when it calls a g_task_return_ function.
>
> > + g_task_set_task_data (map_drive_task, cancel_map, NULL);
> > + g_task_run_in_thread (map_drive_task, map_drive);
>
> I think you could unref after this call, since run_in_thread() keeps a ref.
>
> Before adding and calling map_drive, I think you should implement the
> function to check if there is already a mapping.
>
Mapping should be always removed before starting the service.
It is removed when stopping the service, and automatically removed when
restarting Windows.
>
> > +#endif
> > +
> > g_socket_service_start (socket_service);
> >
> > cancel = g_cancellable_new ();
> > @@ -775,6 +852,10 @@ run_service (void)
> > g_main_loop_run (loop);
> > g_main_loop_unref (loop);
> >
> > +#ifdef G_OS_WIN32
> > + g_cancellable_cancel (cancel_map);
>
> no unref either?
>
Will add the necessary unrefs.
>
> > +#endif
> > +
> > g_cancellable_cancel (cancel);
> >
> > g_clear_object (&mux_istream);
> > --
> > 2.5.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau
>
--
Lukas Venhoda
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
