[PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle
Maarten Lankhorst
m.b.lankhorst at gmail.com
Thu Oct 16 16:56:34 CDT 2008
Juan Lang schreef:
> Hi Maarten,
>
> overall this patch looks pretty good. A few minor comments:
>
> + tempbuffer = HeapAlloc(GetProcessHeap(), 0, TEMP_BLOCK_SIZE);
> + if (!tempbuffer)
> + goto out;
>
> You return TRUE in this case. You probably ought to set last error
> and return FALSE instead.
>
Oops, thanks for catching that. I think HeapAlloc will already set the
last error correctly, so I don't have to worry about that.
> + while (ReadFile(hFile, tempbuffer, TEMP_BLOCK_SIZE,
> &readbytes, NULL) && readbytes)
> + CryptHashData(hash, tempbuffer, readbytes, 0);
>
> Why not while ((ret = ReadFile...) && readbytes) instead? This way
> you can at least return failure if reading the file fails for whatever
> reason.
>
I'll fix that too, thanks.
> + SetLastError(ERROR_INSUFFICIENT_BUFFER);
> return TRUE;
>
> Why are you setting ERROR_INSUFFICIENT_BUFFER in the success case? If
> it's just to pass that test, I think you might remove the test
> instead: in general we don't like to check last error in case of
> success, unless we know an app does so.
>
It is in the success case where no buffer is allocated (eg it wants to
know the buffer size).
It might be better to just wrap it in an 'else { ... }' to be more
specific about this.
Cheers,
Maarten.
More information about the wine-devel
mailing list