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

[Victor Toso]

Hey,

Sorry, I forgot to reply to this sooner.

On Fri, Apr 08, 2016 at 01:10:43PM +0200, Lukas Venhoda wrote:
> >
> > > @@ -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..
> >
>    -I’m using gchar* so that it points to the same gchar as ServiceData.
>
>    -Since it is set in a different thread, I can’t use gchar, and copy the
> data back to ServiceData.

Why not use the same struct in both contexts and a mutex to avoid concurrency
problems?

Cheers,
  toso

>
> >    GCancellable * cancel_map;
> >  } MapDriveData;
> >
> >  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?
> 
> 
> 
>    -Glib doesn’t support this kind of error code
> 
>    -Usually with Windows error codes we call GetLastError, which returns a
> code,
> 
>    -and pass it to g_win32_error_message() to translate i to a string.
> 
>    -With WNet code, we already have the error code, and need to pass it to
> 
>    -WNetGetLastError, which doesn’t handle strings correctly, and needs
> 
>    -a complete wrapper function for allocation, and reallocation of the
> error string.
> 
> > +  }
> > +
> > +  return;
> 
> No need to return here
> 
> > +//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
> 
>    -My bad. I used a gpointer before, but then I thought, that I will be
> using the structure anyway.
> 
>    -I can either define the structure outside ifdef, and ifdef the
> contents, or fallback to the gpopinter.
> 
>    -I think that gpointer would be better suited.
> 
> 
> 
> Lukas Venhoda