Re: [PATCH] GetUrlCacheEntryInfo*: Handle null dest pointer when length is given

[Rob Shearman]

2008/6/6 Jon Griffiths :
> - if (dwRequiredSize > *lpdwBufferSize)
> - {
> -
>    *lpdwBufferSize = dwRequiredSize;
> - ret = dwRequiredSize > *lpdwBufferSize? FALSE : TRUE;
> - *lpdwBufferSize = dwRequiredSize;
> -
> - if (!ret)
>     SetLastError(ERROR_INSUFFICIENT_BUFFER);
> -
>    return FALSE;
> - else if (!lpCacheEntryInfo)
> - {
> -
>    SetLastError(ERROR_INVALID_PARAMETER);
> -
>    ret = FALSE;
> }
> - *lpdwBufferSize = dwRequiredSize;
> - return TRUE;
> - return ret;
>
> }

Internal functions should return an error code instead of calling
SetLastError. This avoids situations like this:
> @@ -1531,6 +1539,9 @@ BOOL WINAPI GetUrlCacheEntryInfoA(
>              FALSE /* ANSI */))
>          {
>              URLCacheContainer_UnlockIndex(pContainer, pHeader);
> -
>        /* This case is inconsistent w.r.t RetrieveUrlCacheEntryFile */
> -
>        if (!lpCacheEntryInfo && GetLastError() == ERROR_INVALID_PARAMETER)
> -
>            SetLastError(ERROR_INSUFFICIENT_BUFFER);
>        return FALSE;
>    }
>    TRACE("Local File Name: %s\n", debugstr_a(lpCacheEntryInfo->lpszLocalFileName));

[PATCH] GetUrlCacheEntryInfo*: Handle null dest pointer when length is given

[Jon Griffiths]

> Internal functions should return an error code instead of
> calling SetLastError.

Perhaps in a general sense, but URLCache_CopyEntry already did SetLastError(ERROR_INSUFFICIENT_BUFFER): setting it there means the 4 places that call it don't need to the logic to set the 2 different errors themselves.

Having the inconsistent case marked out is also not a bad thing IMO - it wouldn't be as clear that its a special case if the SetLastError logic was after each call to URLCache_CopyEntry and one of four was subtly different.

J

[PATCH] GetUrlCacheEntryInfo*: Handle null dest pointer when length is given

[Jon Griffiths]

> Internal functions should return an error code instead of
> calling SetLastError.

Perhaps in a general sense, but URLCache_CopyEntry already did
SetLastError(ERROR_INSUFFICIENT_BUFFER): setting it there means the
4 places that call it don't need to ^ the logic to set the 2
different errors themselves.

^ "repeat" is probably the word I was looking for there :-)