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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2016-03-18 at 07:21 +0100, Pavel Grunt wrote:
> 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)
Also would you mind adding some comments to this function ? To make it
clear how it gets the free drive and what these masks represent.

Thanks,
Pavel
> > +{
> > +  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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]