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

[Pavel Grunt]

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 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.
> 
> 
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
>  
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160318/4434d303/attachment.html>