Re: [PATCH v3] iphlpapi: Add support for ConvertLengthToIpv4Mask()

[Huw Davies]

On Sun, Mar 18, 2018 at 09:40:46PM +0100, Dagfinn Reiakvam wrote:
> From: Uberdaff

I'm not sure why you're including the From: line, simply leave it out
unless you're sending a patch from somebody else.
> index 97284ed..62437e0 100644
> --- a/dlls/iphlpapi/iphlpapi_main.c
> +++ b/dlls/iphlpapi/iphlpapi_main.c
> @@ -3223,6 +3223,30 @@ DWORD WINAPI ConvertInterfaceNameToLuidW(const WCHAR *name, NET_LUID *luid)
>  }
>
> /******************************************************************
> -
> - ConvertLengthToIpv4Mask (IPHLPAPI.@)
> - */
>
> +DWORD WINAPI ConvertLengthToIpv4Mask(ULONG mask_len, PULONG mask)
> +{
> - if(mask_len > 32)
> - {
> -
>    *mask = INADDR_NONE;
> -
>    return ERROR_INVALID_PARAMETER;
> - }
> -
> - if(mask_len == 0)
> -
>    *mask = 0;
> - else
> - {
> -
>    //IN_ADDR temp;
> -
>    //temp.S_un.S_addr = 0xffffffff << ( 32 - MaskLength);
> -
>    //*Mask = temp.S_un.S_un_b.s_b4 + ( temp.S_un.S_un_b.s_b3 <-< 8 ) + ( temp.S_un.S_un_b.s_b2 << 16 ) + ( temp.S_un.S_un_b.s_b1 << 24 );

Why did you leave this old code in here?  Just get rid of it.  Besides
which c++ comments are not acceptable in Wine.
> -
>    *mask = htonl(~0u << (32 - mask_len));
> - }
> -
> - return NO_ERROR;
>
> +}
> -
>
> +/******************************************************************
> - if_nametoindex (IPHLPAPI.@)
>
> */
>  IF_INDEX WINAPI IPHLP_if_nametoindex(const char *name)
> diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c
> index 873612d..039d503 100644
> --- a/dlls/iphlpapi/tests/iphlpapi.c
> +++ b/dlls/iphlpapi/tests/iphlpapi.c
> @@ -96,6 +96,7 @@ static DWORD (WINAPI *pConvertInterfaceLuidToNameW)(const NET_LUID*,WCHAR*,SIZE_
>  static DWORD (WINAPI *pConvertInterfaceLuidToNameA)(const NET_LUID*,char*,SIZE_T);
>  static DWORD (WINAPI *pConvertInterfaceNameToLuidA)(const char*,NET_LUID*);
>  static DWORD (WINAPI *pConvertInterfaceNameToLuidW)(const WCHAR*,NET_LUID*);
> +static DWORD (WINAPI *pConvertLengthToIpv4Mask)(ULONG,PULONG);
>
> static PCHAR (WINAPI *pif_indextoname)(NET_IFINDEX,PCHAR);
>  static NET_IFINDEX (WINAPI *pif_nametoindex)(const char*);
> @@ -149,6 +150,7 @@ static void loadIPHlpApi(void)
>      pConvertInterfaceLuidToNameW = (void *)GetProcAddress(hLibrary, "ConvertInterfaceLuidToNameW");
>      pConvertInterfaceNameToLuidA = (void *)GetProcAddress(hLibrary, "ConvertInterfaceNameToLuidA");
>      pConvertInterfaceNameToLuidW = (void *)GetProcAddress(hLibrary, "ConvertInterfaceNameToLuidW");
> - pConvertLengthToIpv4Mask = (void *)GetProcAddress(hLibrary, "ConvertLengthToIpv4Mask");
> pif_indextoname = (void *)GetProcAddress(hLibrary, "if_indextoname");
> pif_nametoindex = (void *)GetProcAddress(hLibrary, "if_nametoindex");
> }
>
> @@ -2174,6 +2176,67 @@ static void test_GetUnicastIpAddressTable(void)
>      pFreeMibTable(table);
>  }
>
> +static void test_ConvertLengthToIpv4Mask(void)
> +{
> - DWORD apiReturn;
> - ULONG mask;
> -
> - if (!pConvertLengthToIpv4Mask)
> - {
> -
>  win_skip( "ConvertLengthToIpv4Mask not available\n" );
> -
>  return;
> - }
> -
> - apiReturn = pConvertLengthToIpv4Mask(0, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == 0,
> - "ConvertLengthToIpv4Mask Mask value %x, expected  0x00000000\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(1, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0x80000000),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0x00000080\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(8, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0xff000000),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0x000000ff\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(16, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0xffff0000),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0x0000ffff\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(24, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0xffffff00),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0x00ffffff\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(31, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0xfffffffe),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0xfeffffff\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(32, &mask);
> - ok(apiReturn == NO_ERROR,
> - "ConvertLengthToIpv4Mask returned %d, expected NO_ERROR\n", apiReturn);
> - ok(mask == htonl(0xffffffff),
> - "ConvertLengthToIpv4Mask Mask value %x, expected 0xffffffff\n", mask);
> -
> - apiReturn = pConvertLengthToIpv4Mask(33, &mask);
> - ok(apiReturn ==  ERROR_INVALID_PARAMETER,
> - "ConvertLengthToIpv4Mask(32, &Mask) returned %d, expected ERROR_INVALID_PARAMETER\n",
> - apiReturn);
> - ok(mask == INADDR_NONE,
> - "ConvertLengthToIpv4Mask Mask value %x, expected  INADDR_NONE\n", mask);

This last test would be more convincing if mask were initialized to
0xdeadbeef (or something).  The result of the previous test sets
it to 0xffffffff which is INADDR_NONE.

However, it probably would be better to rewrite this sequence of tests
as a loop (from 0 to 33) calculating the expected mask in each case.

Huw.