[Spice-devel] [phodav PATCH 5/7 v3] spice-webdavd-windows: Automount shared folder

[Pavel Grunt]

Hi Lukas,

On Thu, 2016-03-17 at 14:47 +0100, Lukas Venhoda wrote:
> Try to connect to shared folder automatically on Windows.
> 
> On each loop of run_service(), run a GTask, that waits for 0.5s.
> If sharing is disabled, read_thread will immediately return and cause
> the main_loop to stop.
> This in turn causes the cancellable object to be canceled and the
> loop stops.
> 
> If nothing happens for 0.5s (no cancel is called), it means, that
> sharing is enabled,
> because read_thread (and main loop) is blocked, and we can call
> map_drive.
> 
> map_drive then enumerates free drive letters, and maps Spice Folder
> to highest possible
> (from Z: to A:).
> 
> If all drive letters are already assigned, show a critical error, but
> don't stop the service. The folder can still be accessed trough other
> means.
> ---
> Changes since v2:
>  - Changed for loop to gpoll
>  - split map_drive into 2 functions
>  - added enum for return values of map_drive
>  - added TODO for renaming the drive
>  - added explanation for the 0.5 delay into commit log
> 
> Changes since v1:
>  - Changed GThread to a GTask
>  - Only wait half a second, instead of 5
> 
> Changes since RFC:
>  - Calling WNetAddConnection2() blindly was slow and caause many
> threads to spawn.
>     - Now only call it once, when it is sure, that it will connect.
>  - Now connects to a * drive, instead of always Z:
>  - Thread is now spawned and joined every time run_service() is
> called.
> ---
>  Makefile.am           |   4 ++
>  spice/spice-webdavd.c | 112
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6127f93..d8e2d53 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -87,6 +87,10 @@ spice_webdavd_LDADD =		\
>  	$(PIE_LDFLAGS)		\
>  	$(NULL)
> 
> +if OS_WIN32
> +spice_webdavd_LDADD += -lnetapi32 -lmpr
> +endif
> +
>  deps.txt:
>  	$(AM_V_GEN)rpm -qa | grep $(host_os) | sort | unix2dos > $@
> 
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index f9c5cf1..49b1884 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -737,11 +737,118 @@ open_mux_path (const char *path)
>    mux_queue = output_queue_new (G_OUTPUT_STREAM (mux_ostream));
>  }
> 
> +#ifdef G_OS_WIN32
> +typedef enum Tmap_drive_enum
> +{
> +  MAP_DRIVE_OK,
> +  MAP_DRIVE_TRY_AGAIN,
> +  MAP_DRIVE_ERROR
> +} map_drive_enum;
> +
> +gchar drive_letter;
You don't need this variable. map_drive can accept const gchar instead
of void.

> +
> +static gchar
> +get_free_drive_letter(void)
> +{
> +  const guint32 max_mask = 1 << 25;
> +  guint32 drives;
> +  guint32 mask;
> +  gint i;
> +
> +  drives = GetLogicalDrives ();
> +
> +  for (i = 0; i < 26; i++)
> +  {
> +      mask = max_mask >> i;
> +      if ((drives & mask) == 0)
> +        return 'z' - i;
> +  }
> +
> +  return 0;
> +}
> +
> +static map_drive_enum
> +map_drive(void)
> +{
> +  gchar local_name[3];
> +  gchar remote_name[] = "http://localhost:9843/";
it is only used in the net_resource
> +  NETRESOURCE net_resource;
> +  guint32 retval;
> +
> +  local_name[0] = drive_letter;
drive_letter should be parameter to this function
> +  local_name[1] = ':';
> +  local_name[2] = 0;
I would prefer some sprintf-like function instead
> +
> +  net_resource.dwType = RESOURCETYPE_DISK;
> +  net_resource.lpLocalName = local_name;
> +  net_resource.lpRemoteName = remote_name;
> +  net_resource.lpProvider = NULL;

In my opinion setting up net_resource should go to a separate
function,. You don't need 'local_name' and 'remote_name' to map the
drive, you need the net_resource.
> +
> +  retval = WNetAddConnection2 (&net_resource, NULL, NULL,
> CONNECT_TEMPORARY);
> +
> +  if (retval == NO_ERROR) {
> +    g_debug ("map_drive ok");
> +    return MAP_DRIVE_OK;
> +  } else if (retval == ERROR_ALREADY_ASSIGNED) {
> +    g_debug ("map_drive already asigned");
> +    return MAP_DRIVE_TRY_AGAIN;
> +  } else {
> +    g_critical ("map_drive error %d", retval);
Is it critical enough to exit the program? I would use g_warning
> +    return MAP_DRIVE_ERROR;
> +  }
> +}
> +
> +static void
> +map_drive_cb(GTask *task,
> +          gpointer source_object,
> +          gpointer task_data,
> +          GCancellable *cancellable)
> +{
> +  guint32 timeout = 500; //half a second
> +  GCancellable * cancel_map = task_data;
> +  GPollFD cancel_pollfd;
> +
> +  if (!g_cancellable_make_pollfd (cancel_map, &cancel_pollfd))
> +  {
> +    g_critical ("GPollFD failed to create.");
critical/warning
> +    return;
> +  }
> +
> +  if (g_poll (&cancel_pollfd, 1, timeout) <= 0)
> +  {
> +    while (TRUE)
> +    {
> +      if ((drive_letter = get_free_drive_letter ()) == 0)
I would move assignment from the condition
> +      {
> +        g_critical ("all drive letters already assigned.");
critical/warning
> +        break;
> +      }
> +
> +      if (map_drive () != MAP_DRIVE_TRY_AGAIN)
> +        break;
> +    }
> +
> +    //TODO: Rename network drive after mapping
Can you please add more info? What is the name ?
> +  }
> +
> +  g_cancellable_release_fd (cancel_map);
> +  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);
> +  g_task_set_task_data (map_drive_task, cancel_map, NULL);
> +  g_task_run_in_thread (map_drive_task, map_drive_cb);
> +  g_object_unref (map_drive_task);
> +#endif
> +
>    g_socket_service_start (socket_service);
> 
>    cancel = g_cancellable_new ();
> @@ -775,6 +882,11 @@ run_service (void)
>    g_main_loop_run (loop);
>    g_main_loop_unref (loop);
> 
> +#ifdef G_OS_WIN32
> +  g_cancellable_cancel (cancel_map);
> +  g_object_unref (cancel_map);
> +#endif
> +
>    g_cancellable_cancel (cancel);
> 
>    g_clear_object (&mux_istream);
> --
> 2.5.0
> 

Thanks,
Pavel