Hi,
On Fri, 2016-03-18 at 08:53 +0100, Lukas Venhoda wrote:
> Hi
>
> Answers are below
>
> Lukáš Venhoda
>
> From: Pavel Grunt
> Sent: pátek 18. března 2016 7:21
> To: Lukas Venhoda; 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.
>
>
I don't see unmap_drive in your patch. If you need it for something in
the future, then change it in future :)
> > +
> > +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
It depends on env setting, see
https://developer.gnome.org/glib/stable/glib-Message-Logging.html#g-cri
tical
Also consider that there is no g_critical in currect codebase
> > + 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
