[Spice-devel] [phodav PATCH 1/3 v4] spice-webdavd-windows: Automount shared folder
Victor Toso
lists at victortoso.com
Wed Mar 30 13:43:42 UTC 2016
On Thu, Mar 24, 2016 at 04:39:40PM +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 warning, but
> don't stop the service. The folder can still be accessed trough other
> means.
> ---
> Changes since v3:
> - Added init_netresource and clear_netresource functions
> - Should clean up the map_drive function
> - Better handeling of adress to map/unmap
> - Not hardcoded port
> - Syntax cleanup
> - Changed criticals to warnings
> - Better TODO
>
> 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 | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 123 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..ee49469 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -737,11 +737,125 @@ open_mux_path (const char *path)
> mux_queue = output_queue_new (G_OUTPUT_STREAM (mux_ostream));
> }
>
> +#ifdef G_OS_WIN32
> +typedef enum TMapDriveEnum
> +{
> + MAP_DRIVE_OK,
> + MAP_DRIVE_TRY_AGAIN,
> + MAP_DRIVE_ERROR
> +} MapDriveEnum;
> +
> +static gchar
> +get_free_drive_letter(void)
> +{
> + const guint32 max_mask = 1 << 25;
> + guint32 drives;
> + guint32 mask;
You can define mask inside the for
> + gint i;
> +
> + drives = GetLogicalDrives ();
I think we can check for errors here?
"If the function fails, the return value is zero. To get extended error
information, call GetLastError."
> +
> + for (i = 0; i < 26; i++)
> + {
> + mask = max_mask >> i;
> + if ((drives & mask) == 0)
> + return 'z' - i;
> + }
> +
> + return 0;
> +}
> +
> +/* User is required to call clear_netresource, when no longer needed. */
> +static void
> +init_netresource(NETRESOURCE * net_resource, const gchar drive_letter)
> +{
> + net_resource->dwType = RESOURCETYPE_DISK;
> + net_resource->lpLocalName = g_strdup_printf("%c:", drive_letter);
> + net_resource->lpRemoteName = g_strdup_printf("http://localhost:%d/", port);
> + net_resource->lpProvider = NULL;
> +}
> +
> +static void
> +clear_netresource(NETRESOURCE * net_resource)
> +{
> + g_free(net_resource->lpLocalName);
> + g_free(net_resource->lpRemoteName);
> +}
As for the naming I suggest netresource_(init free)
> +
> +static MapDriveEnum
> +map_drive(const gchar drive_letter)
> +{
> + NETRESOURCE net_resource;
> + guint32 errn;
> +
> + init_netresource(&net_resource, drive_letter);
> + errn = WNetAddConnection2 (&net_resource, NULL, NULL, CONNECT_TEMPORARY);
> + clear_netresource(&net_resource);
> +
> + if (errn == NO_ERROR) {
> + g_debug ("map_drive ok");
> + return MAP_DRIVE_OK;
> + } else if (errn == ERROR_ALREADY_ASSIGNED) {
> + g_debug ("map_drive already asigned");
> + return MAP_DRIVE_TRY_AGAIN;
> + } else {
> + g_warning ("map_drive error %d", errn);
> + 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;
> + gchar drive_letter;
> + GPollFD cancel_pollfd;
> +
> + if (!g_cancellable_make_pollfd (cancel_map, &cancel_pollfd)) {
> + g_critical ("GPollFD failed to create.");
> + return;
> + }
> +
> + if (g_poll (&cancel_pollfd, 1, timeout) <= 0)
IIUC, shouldn't this be the opposite? You want to enter in the loop
bellow in case the timeout is NOT reached. g_poll returns -1 on
int/error and 0 when timed out.
gint ret = g_poll (&cancel_pollfd, 1, timeout);
g_cancellable_release_fd (cancel_map);
if (ret <= 0)
return;
while (TRUE)
{
...
> + {
> + while (TRUE)
> + {
> + drive_letter = get_free_drive_letter ();
> + if (drive_letter == 0) {
> + g_warning ("all drive letters already assigned.");
> + break;
> + }
> +
> + if (map_drive (drive_letter) != MAP_DRIVE_TRY_AGAIN) {
> + break;
> + }
Also, MAP_DRIVE_TRY_AGAIN really means 'ERROR_ALREADY_ASSIGNED' so, the
'get_free_drive_letter' returned a non free letter to assign as drive.
As next interaction, I don't see a reason why GetLogicalDrives() would
return different bitmask. Maybe you get_free_drive_letter could take
last letter as an argument if this situation really happens.
> + }
> +
> + //TODO: After mapping, rename network drive from \\localhost at PORT\DavWWWRoot to something like SPICE Shared Folder
Break the line and file a bug about this TODO ;)
> + }
> +
> + 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
> +
I was wondering if g_task_set_return_on_cancel here and
g_usleep (timeout) in the thread could make this code a
bit simpler but they way you did seems fine to me.
> g_socket_service_start (socket_service);
>
> cancel = g_cancellable_new ();
> @@ -775,6 +889,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.5
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Cheers,
toso
More information about the Spice-devel
mailing list