[Spice-devel] [phodav PATCH 3/3 v7 spice-webdavd-windows: Dismount shared folder on service stop

[Victor Toso]

Hi,

On Wed, May 25, 2016 at 03:51:44PM +0200, Lukáš Venhoda wrote:
> From: Lukas Venhoda <lvenhoda at redhat.com>
>
> When stopping the service, automatically disconnect shared folder on
> windows. Not dismounting could lead to multiple shared folders.
> ---
> Changes since v6:
>  - Changed gchar* to ServiceData*
>  - Added mutex for ServiceData multithread handeling
>  - Moved ServiceData from windows service only to all OS
>     - ServiceData is currently empty on Linux
>     - Can be used in the future
>  - Added correct unmapping for --no-service

Great.

>
> Changes since v5:
>  - fixed indentation
>  - fixed * indentaion
>  - removed return from void functions
>  - changed ServiceData * to gpointer, so that it compiles on linux
>
> Changes since v4:
>  - Dont lookup drive letter when unmapping
>     - Uses a service_data structure and map_drive_data structure to store
>       the drive_letter for later use while unmapping
>  - Better debug messages
>
> Changes since v3:
>  - Better handeling of string names
>  - Syntax cleanup
>  - Remove global variable drive_letter
>     - Now scans for mapped drive and unmaps it
> 
> Changes since v2:
>  - None
> 
> Changes since v1:
>  - New patch
> ---
>  spice/spice-webdavd.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index 216d895..0e1081e 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -57,6 +57,14 @@ typedef struct _OutputQueueElem
>    gpointer      user_data;
>  } OutputQueueElem;
> 
> +typedef struct _ServiceData
> +{
> +#ifdef G_OS_WIN32
> +  gchar drive_letter;
> +  GMutex mutex;
> +#endif
> +} ServiceData;
> +

Documentation states: "If a GMutex is placed in other contexts (eg:
embedded in a struct) then it must be explicitly initialised using
g_mutex_init()." I think you are missing g_mutex_init() and
g_mutex_clear()

>  static GCancellable *cancel;
> 
>  static OutputQueue*
> @@ -749,6 +757,7 @@ typedef enum _MapDriveEnum
> 
>  typedef struct _MapDriveData
>  {
> +  ServiceData *service_data;
>    GCancellable *cancel_map;
>  } MapDriveData;
> 
> @@ -855,6 +864,31 @@ map_drive(const gchar drive_letter)
>  }
> 
>  static void
> +unmap_drive(const gchar drive_letter)
> +{
> +  gchar local_name[MAX_DRIVE_LETTER_SIZE];
> +  guint32 errn;
> +
> +  g_snprintf(local_name, MAX_DRIVE_LETTER_SIZE, "%c:", drive_letter);
> +  errn = WNetCancelConnection2(local_name, CONNECT_UPDATE_PROFILE, TRUE);
> +
> +  if (errn == NO_ERROR)
> +    {
> +      g_debug ("Shared folder unmapped succesfully");
> +    }
> +  else if (errn == ERROR_NOT_CONNECTED)
> +    {
> +      g_debug ("Drive %c is not connected", drive_letter);
> +    }
> +  else
> +    {
> +      g_warning ("map_drive error %d", errn);
> +    }
> +
> +  return;
> +}
> +
> +static void
>  map_drive_cb(GTask *task,
>               gpointer source_object,
>               gpointer task_data,
> @@ -896,11 +930,16 @@ map_drive_cb(GTask *task,
>        //TODO: After mapping, rename network drive from \\localhost at PORT\DavWWWRoot
>        //      to something like SPICE Shared Folder
>      }
> +
> +  g_mutex_lock(&map_drive_data->service_data->mutex);
> +  map_drive_data->service_data->drive_letter = drive_letter;
> +  g_mutex_unlock(&map_drive_data->service_data->mutex);
>  }
> +
>  #endif
> 
>  static void
> -run_service (void)
> +run_service (ServiceData *service_data)
>  {
>    g_debug ("Run service");
> 
> @@ -909,6 +948,11 @@ run_service (void)
>    map_drive_data.cancel_map = g_cancellable_new ();
>    gchar drive_letter = get_spice_folder_letter ();
> 
> +  g_mutex_lock(&service_data->mutex);
> +  service_data->drive_letter = drive_letter;
> +  map_drive_data.service_data = service_data;
> +  g_mutex_unlock(&service_data->mutex);
> +
>    if (drive_letter == 0)
>      {
>        GTask *map_drive_task = g_task_new (NULL, NULL, NULL, NULL);
> @@ -990,10 +1034,15 @@ service_ctrl_handler (DWORD ctrl, DWORD type, LPVOID data, LPVOID ctx)
>  {
>    DWORD ret = NO_ERROR;
> 
> +  ServiceData *service_data = ctx;
> +
>    switch (ctrl)
>      {
>      case SERVICE_CONTROL_STOP:
>      case SERVICE_CONTROL_SHUTDOWN:
> +        g_mutex_lock(&service_data->mutex);
> +        unmap_drive (service_data->drive_letter);
> +        g_mutex_unlock(&service_data->mutex);
>          quit (SIGTERM);
>          service_status.dwCurrentState = SERVICE_STOP_PENDING;
>          SetServiceStatus (service_status_handle, &service_status);
> @@ -1009,8 +1058,10 @@ service_ctrl_handler (DWORD ctrl, DWORD type, LPVOID data, LPVOID ctx)
>  VOID WINAPI
>  service_main (DWORD argc, TCHAR *argv[])
>  {
> +  ServiceData service_data;
> +
>    service_status_handle =
> -    RegisterServiceCtrlHandlerEx ("spice-webdavd", service_ctrl_handler, NULL);
> +    RegisterServiceCtrlHandlerEx ("spice-webdavd", service_ctrl_handler, &service_data);
> 
>    g_return_if_fail (service_status_handle != 0);
> 
> @@ -1024,7 +1075,7 @@ service_main (DWORD argc, TCHAR *argv[])
>    SetServiceStatus (service_status_handle, &service_status);
> 
>    while (!quit_service) {
> -      run_service ();
> +      run_service (&service_data);
>        g_usleep (G_USEC_PER_SEC);
>    }
> 
> @@ -1048,6 +1099,7 @@ static GOptionEntry entries[] = {
>  int
>  main (int argc, char *argv[])
>  {
> +  ServiceData service_data;
>    GOptionContext *opts;
>    GError *error = NULL;
> 
> @@ -1110,10 +1162,14 @@ main (int argc, char *argv[])
>      } else
>  #endif
>    while (!quit_service) {
> -    run_service ();
> +    run_service (&service_data);
>      g_usleep (G_USEC_PER_SEC);
>    }
> 
> +#ifdef G_OS_WIN32
> +  unmap_drive (service_data.drive_letter);
> +#endif
> +


I guess we could check if it was succesfully mounted before calling
unmap_drive? Maybe you can initialize it with 0 and just check if its
not zero.

Other than that, looks good.
Reviewed-by: Victor Toso <victortoso at redhat.com>

@list, we did some tests and Lukas is opening a few bugs upstream that
we might want to address later on but the auto mount seems to work well.

Cheers,
  toso

>    g_clear_object (&socket_service);
> 
>    return 0;
> --
> 2.5.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel