Bugzilla – Bug 639
implementation of Buffer::CopyData(std::ostream* os, uint32_t size) is not correct
Last modified: 2009-08-13 03:40:39 UTC
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). Making the size parameter limit the size of the write operation is desirable, e.g., to implement a capture size limit for PCAP traces.
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