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

[Pavel Grunt <pgrunt@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [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-critical

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel