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

[Lukas Venhoda]

Hi



Answers are below



Lukáš Venhoda



*From: *Pavel Grunt <pgrunt at redhat.com>
*Sent: *pátek 18. března 2016 7:21
*To: *Lukas Venhoda <lvenhoda at redhat.com>; spice-devel at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160318/3b342947/attachment.html>