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

[Rob Shearman]

2008/6/6 Jon Griffiths <jon_p_griffiths(a)yahoo.com>:
> -    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));

-- 
Rob Shearman

[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 :-)