RE: [PATCH] setupapi: Rewrite DiskSpaceList logic using lists.

[Jefferson Carpenter]

-typedef struct {
-    WCHAR   lpzName[20];
-    LONGLONG dwFreeSpace;
-    LONGLONG dwWantedSpace;
-} DRIVE_ENTRY, *LPDRIVE_ENTRY;
+struct file_entry
+{
+    struct list entry;
+    WCHAR *path;
+    UINT operation;
+    LONGLONG size;
+};
+
+struct space_list
+{
+    struct list files;
+    UINT flags;
+};

-typedef struct {
-    DWORD   dwDriveCount;
-    DRIVE_ENTRY Drives[26];
-} DISKSPACELIST, *LPDISKSPACELIST;

Until tests are written that constrain the behavior of this module, I'm 
not sure it makes sense to alter the structs here.


-    rc = GetLogicalDriveStringsW(255,drives);
-
-    if (rc == 0)
-        return NULL;
-
-    list = HeapAlloc(GetProcessHeap(),0,sizeof(DISKSPACELIST));
-
-    list->dwDriveCount = 0;
-
-    ptr = drives;
-
-    while (*ptr)
+    list = HeapAlloc(GetProcessHeap(), 0, sizeof(*list));
+    if (list)
      {
-        DWORD type = GetDriveTypeW(ptr);
-        if (type == DRIVE_FIXED)
-        {
-            DWORD clusters;
-            DWORD sectors;
-            DWORD bytes;
-            DWORD total;
-            lstrcpyW(list->Drives[list->dwDriveCount].lpzName,ptr);
-            GetDiskFreeSpaceW(ptr,§ors,&bytes,&clusters,&total);
-            list->Drives[list->dwDriveCount].dwFreeSpace = clusters * 
sectors *
-                                                           bytes;
-            list->Drives[list->dwDriveCount].dwWantedSpace = 0;
-            list->dwDriveCount++;
-        }
-       ptr += lstrlenW(ptr) + 1;
+        list->flags = flags;
+        list_init(&list->files);
      }
+

Likewise, I see no reason to alter this behavior until there are tests 
for it.


+struct file_entry
+{
+    struct list entry;
+    WCHAR *path;
+    UINT operation;
+    LONGLONG size;
+};
+
+struct space_list
+{
+    struct list files;
+    UINT flags;
+};

If flags and operation were a bitfield and enum respectively (instead of 
UINT and UINT) the compiler could aid us in detecting unintended 
implicit conversions so far as they go, and it would be easier to find 
the values that those fields can correctly be assigned to in the source 
code.  Only at API boundaries must the types be precisely the types 
chosen by Microsoft.


-BOOL WINAPI SetupDestroyDiskSpaceList(HDSKSPC DiskSpace)
+BOOL WINAPI SetupDestroyDiskSpaceList(HDSKSPC diskspace)
  {
-    LPDISKSPACELIST list = DiskSpace;
-    HeapFree(GetProcessHeap(),0,list);
+    struct space_list *list = diskspace;
+    struct file_entry *file, *file2;
+
+    if (!diskspace)
+    {
+        SetLastError(ERROR_INVALID_PARAMETER);
+        return FALSE;
+    }
+
+    LIST_FOR_EACH_ENTRY_SAFE(file, file2, &list->files, struct 
file_entry, entry)
+    {
+        HeapFree(GetProcessHeap(), 0, file->path);
+        list_remove(&file->entry);
+        HeapFree(GetProcessHeap(), 0, file);
+    }
+
+    HeapFree(GetProcessHeap(), 0, list);
      return TRUE;
  }

Note this function is also longer after the above modifications to the 
data structures.  You should write tests that confirm the computations 
and effects of SetupAPI, and then write the Wine implementation guided 
by those.


- Jefferson

[PATCH] setupapi: Rewrite DiskSpaceList logic using lists.

[Vijay Kiran Kamuju]

On Mon, Apr 29, 2019 at 2:50 AM Jefferson Carpenter
<jeffersoncarpenter2(a)gmail.com> wrote:
> -typedef struct {
> -    WCHAR   lpzName[20];
> -    LONGLONG dwFreeSpace;
> -    LONGLONG dwWantedSpace;
> -} DRIVE_ENTRY, *LPDRIVE_ENTRY;
> +struct file_entry
> +{
> +    struct list entry;
> +    WCHAR *path;
> +    UINT operation;
> +    LONGLONG size;
> +};
> +
> +struct space_list
> +{
> +    struct list files;
> +    UINT flags;
> +};
>
> -typedef struct {
> -    DWORD   dwDriveCount;
> -    DRIVE_ENTRY Drives[26];
> -} DISKSPACELIST, *LPDISKSPACELIST;
>
> Until tests are written that constrain the behavior of this module, I'm
> not sure it makes sense to alter the structs here.
>
> -    rc = GetLogicalDriveStringsW(255,drives);
> -
> -    if (rc == 0)
> -        return NULL;
> -
> -    list = HeapAlloc(GetProcessHeap(),0,sizeof(DISKSPACELIST));
> -
> -    list->dwDriveCount = 0;
> -
> -    ptr = drives;
> -
> -    while (*ptr)
> +    list = HeapAlloc(GetProcessHeap(), 0, sizeof(*list));
> +    if (list)
>       {
> -        DWORD type = GetDriveTypeW(ptr);
> -        if (type == DRIVE_FIXED)
> -        {
> -            DWORD clusters;
> -            DWORD sectors;
> -            DWORD bytes;
> -            DWORD total;
> -            lstrcpyW(list->Drives[list->dwDriveCount].lpzName,ptr);
> -            GetDiskFreeSpaceW(ptr,§ors,&bytes,&clusters,&total);
> -            list->Drives[list->dwDriveCount].dwFreeSpace = clusters *
> sectors *
> -                                                           bytes;
> -            list->Drives[list->dwDriveCount].dwWantedSpace = 0;
> -            list->dwDriveCount++;
> -        }
> -       ptr += lstrlenW(ptr) + 1;
> +        list->flags = flags;
> +        list_init(&list->files);
>       }
> +
>
> Likewise, I see no reason to alter this behavior until there are tests
> for it.
>
> +struct file_entry
> +{
> +    struct list entry;
> +    WCHAR *path;
> +    UINT operation;
> +    LONGLONG size;
> +};
> +
> +struct space_list
> +{
> +    struct list files;
> +    UINT flags;
> +};
>
> If flags and operation were a bitfield and enum respectively (instead of
> UINT and UINT) the compiler could aid us in detecting unintended
> implicit conversions so far as they go, and it would be easier to find
> the values that those fields can correctly be assigned to in the source
> code.  Only at API boundaries must the types be precisely the types
> chosen by Microsoft.
>
> -BOOL WINAPI SetupDestroyDiskSpaceList(HDSKSPC DiskSpace)
> +BOOL WINAPI SetupDestroyDiskSpaceList(HDSKSPC diskspace)
>   {
> -    LPDISKSPACELIST list = DiskSpace;
> -    HeapFree(GetProcessHeap(),0,list);
> +    struct space_list *list = diskspace;
> +    struct file_entry *file, *file2;
> +
> +    if (!diskspace)
> +    {
> +        SetLastError(ERROR_INVALID_PARAMETER);
> +        return FALSE;
> +    }
> +
> +    LIST_FOR_EACH_ENTRY_SAFE(file, file2, &list->files, struct
> file_entry, entry)
> +    {
> +        HeapFree(GetProcessHeap(), 0, file->path);
> +        list_remove(&file->entry);
> +        HeapFree(GetProcessHeap(), 0, file);
> +    }
> +
> +    HeapFree(GetProcessHeap(), 0, list);
>       return TRUE;
>   }
>
> Note this function is also longer after the above modifications to the
> data structures.  You should write tests that confirm the computations
> and effects of SetupAPI, and then write the Wine implementation guided
> by those.
>
> - Jefferson

This reimplementation is needed for implementation of
SetupAddToDiskSpaceList and related functions.