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 
> Date: Sun, 14 Feb 2016 17:27:37 +0800
> Subject: [PATCH] packet: create packet.dll and Implement GetAdapterNames
> Signed-off-by: Jianqiu Zhang
>
> 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