Bug 639

Created attachment 531 [details]
proposed fix

This patch in my opinion fixes the problem.

(In reply to comment #0)
> PcapWriter writes packet data to file using Buffer::CopyData(std::ostream *os,
> uint32_t size). The function interface is not documented. One would expect that
> the size parameter allows the caller to specify the maximum amount of
> bytes to copy. However, this is not the actual behavior. 
> 
> Looking at the implementation of the function, I can't really understand the
> reason for this check: 
> if (size == GetSize ())
> 
> The only code calling this method is PcapWriter (which calls it through
> Packet::CopyData(std::ostream *os, uint32_t size)), I could see no other use of
> this function. PcapWriter passes packet->.GetSize () as the value of the size
> parameter, so the above check is always true, and the "else" part of the
> statement is never executed. Furthremore, this "else" part does not limit the
> size of the write operation (I wonder what it was meant to do).

the 'else' part is missing a size--; statement to write only the requested amount of bytes

> 
> Making the size parameter limit the size of the write operation is desirable,
> e.g., to implement a capture size limit for PCAP traces.

exactly. I was planning to an a MaxSize attribute to the PcapWriter to allow users control over that per-packet limit.

If you tested your patch better than the current code was tested, feel free to push it.

> the 'else' part is missing a size--; statement to write only the requested
> amount of bytes

That's the first thing I tried, but for some reason it didn't work... 

> > Making the size parameter limit the size of the write operation is desirable,
> > e.g., to implement a capture size limit for PCAP traces.
> 
> exactly. I was planning to an a MaxSize attribute to the PcapWriter to allow
> users control over that per-packet limit.

I implemented this functionality, see the new patch below.

> 
> If you tested your patch better than the current code was tested, feel free to
> push it.
> 

I tested my fix for Buffer::CopyData() with the new limited PCAP capture size functionality and it works. If both patches are ok for you, I will commit both of them (in two separate changesets).

Nicola

Created attachment 536 [details]
configurable PCAP capture size limit

Comment on attachment 536 [details]
configurable PCAP capture size limit

>diff -r 409406fcd1a4 src/common/pcap-writer.cc
>--- a/src/common/pcap-writer.cc	Wed Jul 15 18:44:16 2009 +0200
>+++ b/src/common/pcap-writer.cc	Wed Jul 15 18:50:53 2009 +0200
>@@ -46,6 +46,7 @@
> };
> 
> PcapWriter::PcapWriter ()
>+  : m_captureSize(0)

I was hoping you would make PcapWriter derive from the Object base class, define a TypeId, and define an attribute CaptureSize so that you don't need to bother with defining YansWifiPhyHelper::SetPcapCaptureSize. i.e., you could easily set the pcap capture size from the command-line:

--ns3::PcapWriter::CaptureSize=100

changesets b0743dbc4e55 and b251fb79becb