So just to be clear: should I, or shouldn't I, implement the solution I suggest below and submit a patch?
-----Original Message----- From: Shaked, Nitzan Sent: Thursday, March 19, 2009 2:57 PM To: tcpdump-workers@lists.tcpdump.org Subject: RE: [tcpdump-workers] Bug in libpcap: savefile.c / get_selectable_fd() Cool. I appreciate the comments, and agree to pretty much most of them. What's next? Realistically speaking, should I hold my breath for those changes? (Should I implement them and submit a patch?) One comment: while strictly speaking what you write below about the changes having to be done even now if I want to read from a pipe with select(), in reality calling setvbuf() on the FILE* works great for me. That's because I read from a pipe, with tcpdump -w writing to it. So if there is SOMETHING in the pipe, even if it's only half a header or something like that, I'm guaranteed that the rest will follow shortly (I run tcpdump -U -w). Since the fread()'s in sf_next_packet() are blocking, I don't have to change the code AND it returns quickly enough. Thanks again, Nitzan -----Original Message----- From: tcpdump-workers-ow...@lists.tcpdump.org [mailto:tcpdump-workers-ow...@lists.tcpdump.org] On Behalf Of Guy Harris Sent: Thursday, March 19, 2009 12:34 PM To: tcpdump-workers@lists.tcpdump.org Subject: Re: [tcpdump-workers] Bug in libpcap: savefile.c / get_selectable_fd() On Mar 19, 2009, at 2:39 AM, Shaked, Nitzan wrote: > 2) I believe the current code doesn't have the notion of "non- > blocking", > which it doesn't have the notion of "I read only part of what was > request, maybe half a packet, or half a header", ...and it would even need that if you got rid of buffering in the savefile code, as, when select() says the descriptor is readable, there's no guarantee that what will be readable from the pipe will be a collection of one or more full records with no fraction of a record at the end. > 1) Encapsulation. From your suggestion to put the fd in non-blocking > mode, I understand you mean to actually call fcntl() myself (not from > inside libpcap, since pcap_setnonblock() does nothing on savefiles). > This means a flow which goes something like this: > > if ( is_tty(filename) || is_pipe(filename) || > is_socket(filename) ... ) > { > fp = fopen( ... ); > fcntl( fp, ... ); (fcntl() doesn't work on FILE *'s, it works on file descriptors, so you'd have to call fileno().) > pcap_fopen_offline( fp, ... ); > } > > ... I can't use pcap_open_offline(), since it will do its own fopen() > and I'll lose the non-blocking effect. No, you'd call pcap_file() and then call fileno() and call fcntl() on that. Or, given that, as indicated above, in order to use select() when reading from a pipe we'd have to change the savefile code *anyway*, we could make pcap_setnonblock() to work on savefiles. > So in effect pcap_open_offline() > becomes redundant. Everybody will have to know about the special > non-blocking thing, do the logic I just mentioned above, and call > pcap_fopen_offline(). Everybody *who needs to use select()* would do so. *NOT* all applications using pcap_open_offline() need to use select(); tcpdump, for example, doesn't. > 2) Bug (?) in the current code. In savefile.c, in > pcap_fopen_offline(), > there's an fread() to read the savefile's header. The fread goes > something like "amt_read = fread( &hdr, 1, sizeof(hfr), fp )", and > then > asking "if ( amt_read != sizeof(hdr) )". So we're asking to read > sizeof(hdr) chars (portability issue: not all platforms have 8bit > chars). (I'll wait until somebody has a version of libpcap() running on TOPS-10 or TOPS-20 or Unix-on-Univac-mainframes or... before we worry about that. Programs using libpcap/WinPcap would have plenty of portability issues on those platforms over and above any portability issues libpcap has; all the current platforms on which libpcap runs have 8-bit bytes.) > If we're in non-blocking mode, and reading from a pipe, say, it > could be that there are only 4 bytes available right now, but there > will > be more available later. If we're in blocking mode, the same could be true, as indicated above. If you really want to be able to use libpcap to read from a pipe, without blocking indefinitely on the pipe, with the aid of select(), the code *has* to be able to deal with that, even if it does only one-byte read() calls, as select() might tell you that there's only one byte to read. > 4) Regarding the SF_ERR_AGAIN I mention above, and regarding the > translation of return values from sf_next_packet() and > pcap_offline_read() I mention: once we introduce the notion of > non-blocking IO, it seems to me that we need to introduce new return > values all across: > > 4.1) sf_next_packet() should return: It can do whatever's necessary, as it's static. > 4.2) pcap_offline_read() should return: ... > OFFLINE_READ_BREAK_LOOP (value: -3), if we were told to break > out of the loop No, that breaks binary compatibility. It'd have to continue to return -2 when told to break out of the loop. - This is the tcpdump-workers list. Visit https://cod.sandelman.ca/ to unsubscribe. - This is the tcpdump-workers list. Visit https://cod.sandelman.ca/ to unsubscribe.
