Hi
Answers are below
Lukáš Venhoda
*From: *Pavel Grunt <pgr...@redhat.com>
*Sent: *pátek 18. března 2016 7:21
*To: *Lukas Venhoda <lvenh...@redhat.com>; spice-devel@lists.freedesktop.org
*Subject: *Re: [Spice-devel] [phodav PATCH 5/7 v3] spice-webdavd-windows:
Automount shared folder
> +gchar drive_letter;
P: You don't need this variable. map_drive can accept const gchar instead
of void.
L: This variable is not because of map_drive, but because of unmap_drive.
I need to remember to what letter I mapped the drive.
I could pass a structure with cancel_map and drive_letter to map_drive_cb
instead.
> +
> +static gchar
> +get_free_drive_letter(void)
P: 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.
L: Sure, I’ll add it as a comment.
Basically GetLogicalDrives() returns bitfield of drives (A-Z … 1-26)
The max_mask is Z, shifting causes going from Z to A.
> +{
> + 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/";;
P: it is only used in the net_resource
L: Ok will change
> + NETRESOURCE net_resource;
> + guint32 retval;
> +
> + local_name[0] = drive_letter;
P: drive_letter should be parameter to this function
L: See previous comment
> + local_name[1] = ':';
> + local_name[2] = 0;
I would prefer some sprintf-like function instead
Ok will change
> +
> + net_resource.dwType = RESOURCETYPE_DISK;
> + net_resource.lpLocalName = local_name;
> + net_resource.lpRemoteName = remote_name;
> + net_resource.lpProvider = NULL;
P: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.
L: Ok will change
> +
> + 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);
P: Is it critical enough to exit the program? I would use g_warning
L: g_critical doesnt exit the program, but no other error then
ALREADY_ASIGNED should happen here
> + 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.");
P: critical/warning
L: See ^
> + return;
> + }
> +
> + if (g_poll (&cancel_pollfd, 1, timeout) <= 0)
> + {
> + while (TRUE)
> + {
> + if ((drive_letter = get_free_drive_letter ()) == 0)
P: I would move assignment from the condition
L: Ok will change
> + {
> + g_critical ("all drive letters already assigned.");
P: critical/warning
L: Warning might be better here
> + break;
> + }
> +
> + if (map_drive () != MAP_DRIVE_TRY_AGAIN)
> + break;
> + }
> +
> + //TODO: Rename network drive after mapping
P: Can you please add more info? What is the name ?
L: Sure
> + }
> +
> + g_cancellable_release_fd (cancel_map);
> + return;
> +}
> +#endif
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
