Re: savefile.c patch

[Guy Harris <guy () alum mit edu>]

On May 26, 2004, at 1:55 AM, Gisle Vanem wrote:

> I feel it's high time we cleanup some of the sources. I'd start
> with savefile.c. Currently it doesn't work for offline data from stdin.
>
> --gv
>
> --- libpcap-2004.05.20/savefile.c       Tue Mar 23 21:18:08 2004
> +++ savefile.c  Wed Mar 24 16:29:06 2004
> @@ -52,6 +52,12 @@
>  #define TCPDUMP_MAGIC 0xa1b2c3d4
>  #define PATCHED_TCPDUMP_MAGIC 0xa1b2cd34
>
> +#if defined(WIN32) || defined(MSDOS)
> +#define SETMODE(file,mode)  setmode(file,mode)
> +#else
> +#define SETMODE(file,mode)  ((void)0)
> +#endif
> +

Perhaps it should just define "SETMODE()" as nothing - that would cause

        SETMODE(file, mode);

to expand to

        ;

which, as a null statement, should be a valid statement.

Also, is "setmode()" sufficient with all the compilers that could be 
used to compile libpcap/WinPcap on Windows (MSVC++, MinGW, etc.), or is 
"_setmode()" needed with some compilers?  (The code currently uses 
"_setmode()".)

> -#ifndef WIN32
> -               fp = fopen(fname, "r");
> -#else
>                 fp = fopen(fname, "rb");
> -#endif

Presumably there are no interesting UN*X platforms left that wouldn't 
ignore the "b" (Ethereal's library for reading capture files 
unconditionally uses "rb"), so that should be OK.

> -       if(fp)
> +       if(fp && fp != stdin)
>                 fclose(fp);
> +       if (fp == stdin)
> +               SETMODE (fileno(stdin),O_TEXT);

Why not just

        if (fp) {
                if (fp == stdin)
                        SETMODE(fileno(stdin), O_TEXT);
                else
                        fclose(fp);
        }

> @@ -973,6 +981,7 @@
>  pcap_dump_open(pcap_t *p, const char *fname)
>  {
>         FILE *f;
> +       pcap_dumper_t *pd;
>         int linktype;
>
>         linktype = dlt_to_linktype(p->linktype);
> @@ -985,26 +994,23 @@
>
>         if (fname[0] == '-' && fname[1] == '\0') {
>                 f = stdout;
> -#ifdef WIN32
> -               _setmode(_fileno(f), _O_BINARY);
> -#endif
> +               SETMODE(fileno(f), O_BINARY);
>         } else {
> -#ifndef WIN32
> -               f = fopen(fname, "w");
> -#else
>                 f = fopen(fname, "wb");
> -#endif
> -               if (f == NULL) {
> +               setbuf(f, NULL);        /* XXX - why? */
> +       }

I'm not sure why we're setting the output unbuffered on Windows; even 
if there's a legitimate reason to do so, I don't see any reason to do 
so on UN*X - we really don't want to have the standard I/O library 
routines make a separate "write()" call for every "fwrite()" etc. call 
to the file.

-
This is the tcpdump-workers list.
Visit https://lists.sandelman.ca/ to unsubscribe.