On Fri, Jul 22, 2016 at 9:04 AM, Philipp Hoppermann plata@mailbox.org wrote:
Replaced call to inet_ntop by more elaborate solution.
Looks much better, thanks. See the comments below.
Signed-off-by: Philipp Hoppermann plata@mailbox.org
dlls/ws2_32/socket.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-)
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 0622060..2045163 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -7934,11 +7934,75 @@ INT WINAPI InetPtonW(INT family, PCWSTR addr, PVOID buffer) }
/*********************************************************************** -* InetNtopW (WS2_32.@) -*/ -PCWSTR WINAPI InetNtopW(INT family, PVOID addr, PWSTR stringBuf, SIZE_T stringBufSize)
InetNtopW (WS2_32.@)
- Get string representation of IP address.
- PARAMS
- family [I] AF_INET or AF_INET6
- addr [I] IP address in byte
- buffer [O] IP address as string
- buffer_len [O] length of the buffer in characters
- RETURNS
- Success: IP address as string
- Failure: NULL. Use GetLastError() to find the error cause.
- Error codes:
WSAEAFNOSUPPORT
- family is not AF_INET or AF_INET6
ERROR_INVALID_PARAMETER
- buffer is NULL
- buffer_len is 0
- buffer_len is too small
(i.e. < 16 for AF_INET or < 46 for AF_INET6)
- */
Usually only overly complex functions need comments, which is not the case here so you can drop them. As you can see in the file most almost no other function has comments like this.
+PCWSTR WINAPI InetNtopW(INT family, PVOID addr, PWSTR buffer, SIZE_T buffer_len) {
- return inet_ntop(family, addr, stringBuf, stringBufSize);
- char *bufferA;
- TRACE("family %d, addr %s, buffer (%p), buffer_len %lu\n", family, debugstr_w(addr), buffer, (unsigned long) buffer_len);
WS_inet_ntop traces buffer_len without the cast using %ld, I guess it is better to follow its example and avoid the cast.
- // check family
// comments are not allowed, please use /* */. But most of them are obvious like "check if buffer is NULL" so they can be dropped.
- if ((family != AF_INET) && (family != AF_INET6))
- {
SetLastError(WSAEAFNOSUPPORT);return NULL;- }
- // check if buffer is NULL
- if (buffer == NULL)
- {
SetLastError(ERROR_INVALID_PARAMETER);return NULL;- }
- // check buffer_len
- if ((family == AF_INET && buffer_len < 16) || (family == AF_INET6 && buffer_len < 46))
- {
SetLastError(ERROR_INVALID_PARAMETER);return NULL;- }
WS_inet_ntop already does some error checking, there is no need to manually test again, also we trust the system inet_ntop call to deal with buffer_len check.
- // allocate memory for the string representation in char
- if (!(bufferA = HeapAlloc(GetProcessHeap(), 0, buffer_len*sizeof(WCHAR))))
- {
SetLastError(WSA_NOT_ENOUGH_MEMORY);return NULL;- }
You are doubling the required amount of memory, buffer_len should be already in characters, so no need to multiply.
- if (WS_inet_ntop(family, addr, bufferA, buffer_len*sizeof(WCHAR)) != NULL)
Again no need to multiply.
- {
// convert to wcharMultiByteToWideChar(CP_ACP, 0, bufferA, buffer_len*sizeof(WCHAR), buffer, buffer_len);
Use: MultiByteToWideChar(CP_ACP, 0, bufferA, -1, buffer, buffer_len);
HeapFree(GetProcessHeap(), 0, bufferA);return buffer;- } else
- {
return NULL;
You are leaking bufferA in the else case, use a variable to store the results of WS_inet_ntop and return it instead so you can always HeapFree before the return and then there is no need for else.
Best wishes, Bruno