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

[Victor Toso]

Hi,

On Thu, Apr 07, 2016 at 05:44:53PM +0200, Lukas Venhoda wrote:
> When stopping the service, automatically disconnect shared folder on
> windows. Not dismounting could lead to multiple shared folders.
> ---
> 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 | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index 571600e..1ccb249 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -749,6 +749,7 @@ typedef enum _MapDriveEnum
> 
>  typedef struct _MapDriveData
>  {
> +  gchar * drive_letter;

I think it is fine to use a gchar instead of gchar* here..

>    GCancellable * cancel_map;
>  } MapDriveData;
> 
> @@ -851,6 +852,26 @@ 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);

I'm guessing we can't get a string for the error?

> +  }
> +
> +  return;

No need to return here

> +}
> +
> +static void
>  map_drive_cb(GTask *task,
>               gpointer source_object,
>               gpointer task_data,
> @@ -888,12 +909,21 @@ map_drive_cb(GTask *task,
>    //TODO: After mapping, rename network drive from \\localhost at PORT\DavWWWRoot
>    //      to something like SPICE Shared Folder
>    }
> +
> +  *(map_drive_data->drive_letter) = drive_letter;
>    return;
>  }
> +
> +typedef struct _ServiceData
> +{
> +  gchar drive_letter;
> +} ServiceData;
> +
>  #endif
> 
> +//Parameter service_data is only used on windows when started as a service
>  static void
> -run_service (void)
> +run_service (ServiceData * service_data G_GNUC_UNUSED)

ServiceData is defined inside the #ifdef windows but it is being used on linux
as well here, making the build fail on linux with

<gcc>
    spice/spice-webdavd.c: At top level:
    spice/spice-webdavd.c:926:14: error: unknown type name ‘ServiceData’
     run_service (ServiceData * service_data G_GNUC_UNUSED)
</gcc>

My suggestion is to create all the structures that you need together before the
functions (top of the file). For the error above, I guess is enough to put the
drive_letter field with #ifdef... or don't. It'll never be used anyway by the
linux daemon.

I'll do some tests later on, I think we need to open a few bugs that could be
closed once that this is integrated with spice-vdagent.

Thanks for your work on this,
  toso

>  {
>    g_debug ("Run service");
> 
> @@ -902,6 +932,13 @@ run_service (void)
>    map_drive_data.cancel_map = g_cancellable_new ();
>    gchar drive_letter = get_spice_folder_letter ();
> 
> +  if (service_data != NULL) {
> +    service_data->drive_letter = drive_letter;
> +    map_drive_data.drive_letter = &(service_data->drive_letter);
> +  } else {
> +    map_drive_data.drive_letter = &(drive_letter);
> +  }
> +
>    if (drive_letter == 0)
>    {
>      GTask * map_drive_task = g_task_new (NULL, NULL, NULL, NULL);
> @@ -983,10 +1020,13 @@ 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:
> +        unmap_drive (service_data->drive_letter);
>          quit (SIGTERM);
>          service_status.dwCurrentState = SERVICE_STOP_PENDING;
>          SetServiceStatus (service_status_handle, &service_status);
> @@ -1002,8 +1042,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);
> 
> @@ -1017,7 +1059,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);
>    }
> 
> @@ -1103,7 +1145,7 @@ main (int argc, char *argv[])
>      } else
>  #endif
>    while (!quit_service) {
> -    run_service ();
> +    run_service (NULL);
>      g_usleep (G_USEC_PER_SEC);
>    }
> 
> --
> 2.5.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel