Re: [PATCH] packet: create packet.dll and Implement GetAdapterNames

[Sebastian Lackner]

I am not sure if André wants to have this dll in wine, but nevertheless, here
some comments.

On 14.02.2016 16:10, Jianqiu Zhang wrote:
> From dfc572cd1b1cdf51b84a5e367d054473c3c3e99d Mon Sep 17 00:00:00 2001
> From: Jianqiu Zhang <zhangjianqiu_133(a)yeah.net>
> Date: Sun, 14 Feb 2016 17:27:37 +0800
> Subject: [PATCH] packet: create packet.dll and Implement GetAdapterNames
>
> Signed-off-by: Jianqiu Zhang <zhangjianqiu_133(a)yeah.net>
> ---
>  dlls/packet/Makefile.in |  5 ++++
>  dlls/packet/packet.c    | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
>  dlls/packet/packet.spec | 32 +++++++++++++++++++++++++

Changes to configure.ac are missing. In this case its especially critical
because compilation of packet.dll would have to be disabled, when pcap header
files are not present. Take a look at how its done for wpcap.dll.
> 3 files changed, 100 insertions(+)
>  create mode 100644 dlls/packet/Makefile.in
>  create mode 100644 dlls/packet/packet.c
>  create mode 100644 dlls/packet/packet.spec
>
> diff --git a/dlls/packet/Makefile.in b/dlls/packet/Makefile.in
> new file mode 100644
> index 0000000..6ab9f08
> --- /dev/null
> +++ b/dlls/packet/Makefile.in
> @@ -0,0 +1,5 @@
> +MODULE    = packet.dll
> +DELAYIMPORTS = iphlpapi

A regular import would make more sense here.
> +
> +C_SRCS = \
> +	packet.c
> diff --git a/dlls/packet/packet.c b/dlls/packet/packet.c
> new file mode 100644
> index 0000000..c6d1568
> --- /dev/null
> +++ b/dlls/packet/packet.c
> @@ -0,0 +1,63 @@
> +#include "config.h"
> +#include "wine/port.h"
> +
> +#include <stdarg.h>
> +
> +#include "windef.h"
> +#include "winbase.h"
> +#include "windows.h"
> +#include "wine/debug.h"
> +
> +#include "winsock2.h"
> +#include "iphlpapi.h"
> +#include "ntddndis.h"
> +
> +#include <pcap/pcap.h>

For the first patch, you will most likely not need all those headers. You should add them
in later patches, whenever you implement a function which needs them.
> +
> +WINE_DEFAULT_DEBUG_CHANNEL(packet);
> +
> +BOOLEAN PacketGetAdapterNames(char *nameList, PULONG pSize)

The calling convention (CDECL, WINAPI) is missing. Also, variable names are
usually lower-case only, and without "p" prefix. Types like P* or LP* should also
be avoided in the wine source.
> +{
> +    IP_ADAPTER_INFO *adInfo, *adInfoList = NULL;
> +    DWORD errcode;
> +    ULONG needsize = 0, minDescLen = 0, minAdNameLen = 0, minTotLen = 0, nameOffset, descOffset;
> +
> +    TRACE("nameList %p &size %p size %d\n", nameList, pSize, *pSize);
> +
> +    errcode = GetAdaptersInfo(adInfoList, &needsize);
> +    adInfoList = HeapAlloc(GetProcessHeap(), 0, needsize);

Its a matter of taste, but checking return values for HeapAlloc functions usually does not hurt.
> +
> +    TRACE("Get the needed space(%d) for adInfo\n", needsize);
> +
> +    errcode = GetAdaptersInfo(adInfoList, &needsize);
> +    if(errcode)
> +    {

You are leaking memory here. The same applies for a couple of other
return paths.
> +        SetLastError(errcode);
> +        return FALSE;
> +    }
> +    for(adInfo = adInfoList; adInfo != NULL; adInfo = adInfo->Next)
> +    {
> +        minAdNameLen += lstrlenA(adInfo->AdapterName) + 1;
> +        minDescLen += lstrlenA(adInfo->Description) + 1;
> +    }
> +    minTotLen = minAdNameLen + minDescLen + 2;
> +    TRACE("minAdNameLen = %d, minDescLen = %d\n", minAdNameLen, minDescLen);

Such a trace is only useful during debugging.
> +    if(nameList == NULL || *pSize < minTotLen)
> +    {
> +        SetLastError(ERROR_INSUFFICIENT_BUFFER);
> +        *pSize = minTotLen;
> +        return FALSE;
> +    }
> +
> +    nameOffset = descOffset = 0;
> +    for(adInfo = adInfoList; adInfo != NULL; adInfo = adInfo->Next)
> +    {
> +        lstrcpyA(nameList + nameOffset, adInfo->AdapterName);
> +        lstrcpyA(nameList + minAdNameLen + descOffset + 1, adInfo->Description);
> +        nameOffset += lstrlenA(adInfo->AdapterName) + 1;
> +        descOffset += lstrlenA(adInfo->Description) + 1;
> +    }
> +    nameList[minAdNameLen] = '\0';
> +    nameList[minTotLen - 1] = '\0';
> +    return TRUE;
> +}
> diff --git a/dlls/packet/packet.spec b/dlls/packet/packet.spec
> new file mode 100644
> index 0000000..b7f2781
> --- /dev/null
> +++ b/dlls/packet/packet.spec
> @@ -0,0 +1,32 @@
> + 1 stub PacketAllocatePacket

Wine does not care about exact ordinal numbers. You can mark them all as "@".
> + 2 stub PacketCloseAdapter
> + 3 stub PacketFreePacket
> + 4 cdecl PacketGetAdapterNames(str long)

The first parameter is a buffer which is filled out by the function, so 'ptr' would make more sense.
The second parameter is a pointer, not long.
> + 5 stub PacketGetAirPcapHandle
> + 6 stub PacketGetDriverVersion
> + 7 stub PacketGetNetInfoEx
> + 8 stub PacketGetNetType
> + 9 stub PacketGetReadEvent
> +10 stub PacketGetStats
> +11 stub PacketGetStatsEx
> +12 stub PacketGetVersion
> +13 stub PacketInitPacket
> +14 stub PacketIsDumpEnded
> +15 stub PacketLibraryVersion
> +16 stub PacketOpenAdapter
> +17 stub PacketReceivePacket
> +18 stub PacketRequest
> +19 stub PacketSendPacket
> +20 stub PacketSendPackets
> +21 stub PacketSetBpf
> +22 stub PacketSetBuff
> +23 stub PacketSetDumpLimits
> +24 stub PacketSetDumpName
> +25 stub PacketSetHwFilter
> +26 stub PacketSetLoopbackBehavior
> +27 stub PacketSetMinToCopy
> +28 stub PacketSetMode
> +29 stub PacketSetNumWrites
> +30 stub PacketSetReadTimeout
> +31 stub PacketSetSnapLen
> +32 stub PacketStopDriver