I'm not the maintainer of wpcap, nevertheless I hope its fine to give some suggestions how to improve the code.
On 11.02.2016 09:55, Jianqiu Zhang wrote:
From 770a1250794833aa7d9a9596a7fc56bfc19c65f0 Mon Sep 17 00:00:00 2001 From: Jianqiu Zhang zhangjianqiu_133@yeah.net Date: Thu, 29 Oct 2015 15:33:28 +0800 Subject: [PATCH 1/2] wpcap: Implement pcap_dump_open and pcap_dump
Signed-off-by: Jianqiu Zhang zhangjianqiu_133@yeah.net
dlls/wpcap/wpcap.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ dlls/wpcap/wpcap.spec | 4 ++-- 2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/dlls/wpcap/wpcap.c b/dlls/wpcap/wpcap.c index ae9e482..e03e18e 100644 --- a/dlls/wpcap/wpcap.c +++ b/dlls/wpcap/wpcap.c @@ -22,6 +22,8 @@ #include "winsock2.h" #include "windef.h" #include "winbase.h" +#include "winternl.h" +#include "ntstatus.h" #include "wine/debug.h"
WINE_DEFAULT_DEBUG_CHANNEL(wpcap); @@ -40,6 +42,8 @@ WINE_DECLARE_DEBUG_CHANNEL(winediag); #define PCAP_SRC_IFLOCAL 3 #endif
+#define MAX_LENGTH 1024
In this case copying is not necessary at all. In other cases you would want to use MAX_PATH instead of own definitions.
void CDECL wine_pcap_breakloop(pcap_t *p) { TRACE("(%p)\n", p); @@ -339,3 +343,53 @@ int CDECL wine_wsockinit(void) if (WSAStartup(MAKEWORD(1,1), &wsadata)) return -1; return 0; }
+pcap_dumper_t* CDECL wine_pcap_dump_open(pcap_t *p, const char *fname) +{
- UNICODE_STRING nt_name, dospathW;
- ANSI_STRING fname_dos;
- ANSI_STRING fname_unix;
- CHAR fpath[MAX_LENGTH] = { 0 };
You don't need this char array (see comment below).
- NTSTATUS res;
Not really critical, but "status" is often a bit more meaningful.
- TRACE("(%p %s)\n", p, fname);
- RtlInitAnsiString(&fname_dos, fname);
- res = RtlAnsiStringToUnicodeString(&dospathW, &fname_dos, TRUE);
- if(res)
Minor issue, but the coding style in this file seems to be with a space between "if" and "(".
- {
SetLastError(RtlNtStatusToDosError(res));return NULL;- }
- if(!RtlDosPathNameToNtPathName_U(dospathW.Buffer, &nt_name, NULL, NULL))
- {
RtlFreeUnicodeString(&dospathW);SetLastError(ERROR_FILENAME_EXCED_RANGE);
Basically all places in wine use ERROR_PATH_NOT_FOUND in this case.
return NULL;- }
- res = wine_nt_to_unix_file_name(&nt_name, &fname_unix, FILE_OPEN_IF, FALSE);
- if(res)
- {
if(res == STATUS_NO_SUCH_FILE)SetLastError(ERROR_SUCCESS);
Setting error code in case of success is not useful.
else{RtlFreeUnicodeString(&dospathW);RtlFreeUnicodeString(&nt_name);SetLastError(RtlNtStatusToDosError(res));return NULL;}- }
- else
- {
SetLastError(ERROR_FILE_EXISTS);
Similar to above, we don't care about the error code in case of success.
- }
- memcpy(fpath, fname_unix.Buffer, fname_unix.Length);
- RtlFreeUnicodeString(&nt_name);
- RtlFreeUnicodeString(&dospathW);
- RtlFreeAnsiString(&fname_unix);
- return pcap_dump_open(p, fpath);
Instead of copying the string, please store the return value of pcap_dump_open in a variable.
+}
+void CDECL wine_pcap_dump(u_char *user, const struct pcap_pkthdr *h, const u_char *sp) +{
- return pcap_dump(user, h, sp);
+} diff --git a/dlls/wpcap/wpcap.spec b/dlls/wpcap/wpcap.spec index 66303b4..541fc49 100644 --- a/dlls/wpcap/wpcap.spec +++ b/dlls/wpcap/wpcap.spec @@ -16,12 +16,12 @@ @ cdecl pcap_datalink_val_to_description(long) wine_pcap_datalink_val_to_description @ cdecl pcap_datalink_val_to_name(long) wine_pcap_datalink_val_to_name @ cdecl pcap_dispatch(ptr long ptr ptr) wine_pcap_dispatch -@ stub pcap_dump +@ cdecl pcap_dump(ptr ptr ptr) wine_pcap_dump @ stub pcap_dump_close @ stub pcap_dump_file @ stub pcap_dump_flush @ stub pcap_dump_ftell -@ stub pcap_dump_open +@ cdecl pcap_dump_open(ptr str) wine_pcap_dump_open @ stub pcap_file @ stub pcap_fileno @ cdecl pcap_findalldevs(ptr ptr) wine_pcap_findalldevs