Re: 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.