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

[Vijay Kiran Kamuju]

On Mon, Apr 29, 2019 at 2:50 AM Jefferson Carpenter
 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,&sectors,&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.