kernel32.dll: implementation of GetVolumePathNamesForVolumeNameA/W (try 2)

[Vitaliy Margolen]

Thanks for the patch. However it has number of issues:

On 07/20/2010 08:33 PM, Tuomo Mattila wrote:
>   @ stdcall GetVolumeNameForVolumeMountPointW(wstr ptr long)
>   @ stdcall GetVolumePathNameA(str ptr long)
>   @ stdcall GetVolumePathNameW(wstr ptr long)
> -# @ stub GetVolumePathNamesForVolumeNameA
> +@ stdcall GetVolumePathNamesForVolumeNameA(str ptr long ptr)
Broken formatting.

> +    /* FIXME: The winxp version of this function raises an exception (access violation) if called with a NULL volumename or volumepathname argument. Alternatively, we might want to set an error code and return a failure */
> +/*
> +    if (volumename == NULL || volumepathname == NULL)
That indicates that you shouldn't be doing any null checks here. Commented 
out code is not accepted into Wine. Also please don't use such long lines, 
especially comments. leek

> +    {
> +        return FALSE;
> +    }
> +*/
> +
> +    volumepathnameW = HeapAlloc(GetProcessHeap(), 0, buflen*sizeof(WCHAR));
> +    if (volumepathnameW == NULL) return FALSE;
This should probably set LastError as well. Need a test to verify.

> +
> +    if ((volumenameW = FILE_name_AtoW(volumename, TRUE)) == 0) return FALSE;
You leaking volumepathnameW here.

> +    WCHAR volumename2string[50];
> +    LPWSTR volumename2 = volumename2string;
Don't do that. Unless you need to do some extra pointer arithmetic don't 
create extra pointer that points to a buffer.

> -    FIXME("(%s, %p, %d, %p), stub!\n", debugstr_w(volumename), volumepathname, buflen, returnlen);
You should replace FIXME with appropriate TRACE.

> +    if (strncmpW(volumename, volname_prefix, 11) != 0 || volumename[19] != '-' || volumename[24] != '-' || volumename[29] != '-' || volumename[34] != '-' || volumename[47] != '}')
You are not checking the length of the volumename. If it's too short you 
accessing it past the end. Also comment about what exactly you are looking 
for and how volume string is supposed to look like will be good.

> +    /* FIXME: combination of the above may set other error codes */
Such as? Need a test for that.

> +        memset(volumename2string, 0, sizeof(volumename2string));
Don't do this sort of thing. There is never a need to clear the entire 
buffer, especially on each iteration.

> +        if (available_drives&  (1<<(letter-'A')))
> +        {
> +            pstr[0] = letter;
> +            if( (GetVolumeNameForVolumeMountPointW(pathstring, volumename2, 50))&&  (!strncmpW(volumename, volumename2, 50)))
Don't use magic numbers. Always use sizeof() when need a buffer size. And as 
I said before you should use volumename2string directly here.

Vitaliy.