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

[Lukas Venhoda]

Hi

On Thu, Feb 25, 2016 at 4:21 PM, Marc-André Lureau <
marcandre.lureau at gmail.com> wrote:

> Hi
>
> On Thu, Feb 25, 2016 at 3:19 PM, Lukas Venhoda <lvenhoda at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau
>

-- 
Lukas Venhoda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160225/054eeeda/attachment.html>