<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"><meta name="Generator" content="Microsoft Word 15 (filtered medium)"><style></style></head><body lang="CS" link="blue" vlink="#954F72"><div class="WordSection1">Hi Answers are below Lukáš Venhoda <div style="border:none;border-top:solid #e1e1e1 1.0pt;padding:3.0pt 0cm 0cm 0cm">From: <a href="mailto:pgrunt@redhat.com">Pavel Grunt</a> Sent: pátek 18. března 2016 7:21 To: <a href="mailto:lvenhoda@redhat.com">Lukas Venhoda</a>; <a href="mailto:spice-devel@lists.freedesktop.org">spice-devel@lists.freedesktop.org</a> Subject: Re: [Spice-devel] [phodav PATCH 5/7 v3] spice-webdavd-windows: Automount shared folder</div> > +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[] = "<a href="http://localhost:9843/" target="_blank">http://localhost:9843/</a>"; P: it is only used in the net_resourceL: Ok will change > + NETRESOURCE net_resource; > + guint32 retval; > + > + local_name[0] = drive_letter; P: drive_letter should be parameter to this functionL: See previous comment > + local_name[1] = ':'; > + local_name[2] = 0; I would prefer some sprintf-like function insteadOk 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_warningL: 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 conditionL: 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 </div></body></html>