Re: [PATCH 1/2] wpcap: Implement pcap_dump_open and pcap_dump(try 2)

[Sebastian Lackner]

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(a)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(a)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