Redo ASCII and pcap Traces

[Mathieu Lacage, 2010-01-29 10:29:29]

nothing to add beyond the small comments below.

http://codereview.appspot.com/196058/diff/1/77
File src/common/output-stream-object.h (right):

http://codereview.appspot.com/196058/diff/1/77#newcode72
src/common/output-stream-object.h:72: class OutputStreamObject : public Object
1) Most likely, you don't need to derive from Object if you just need reference
counting
2) why did you not use the existing AsciiWriter instead of creating this class ?
i.e., why do we need both ?

http://codereview.appspot.com/196058/diff/1/78
File src/common/pcap-file-object.cc (right):

http://codereview.appspot.com/196058/diff/1/78#newcode75
src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
The equivalent code before used to call Packet::CopyData (std::ostream &,
uint32_t) to avoid this extra memory allocation+copy. 

To avoid this now, you need to:
1) add PcapFile::Write (... Ptr<const Packet> p...);
2) fix bug 774
3) call Packet::CopyData (std::ostream &...) from PcapFile::Write defined in 1)
above.

http://codereview.appspot.com/196058/diff/1/79
File src/common/pcap-file-object.h (right):

http://codereview.appspot.com/196058/diff/1/79#newcode42
src/common/pcap-file-object.h:42: bool Open (std::string const &filename,
std::string const &mode);
doxygen missing for all methods.

Also, I am certain that "std::string const &" is not the recommended style for
ns-3. It should be "const std::string &".

http://codereview.appspot.com/196058/diff/1/98
File src/helper/trace-helper.h (right):

http://codereview.appspot.com/196058/diff/1/98#newcode156
src/helper/trace-helper.h:156: Ptr<OutputStreamObject> CreateFileStream
(std::string filename, std::string filemode = "w");
Is there a reason why you did not use/extend AsciiWriter instead of introducing
OutputStreamObject ?

http://codereview.appspot.com/196058/diff/1/102
File src/helper/yans-wifi-helper.h (right):

http://codereview.appspot.com/196058/diff/1/102#newcode242
src/helper/yans-wifi-helper.h:242: void SetPcapDataLinkType (uint32_t dlt);
that dlt should be an enum.

http://codereview.appspot.com/196058/diff/1/106
File src/node/radiotap-header.h (right):

http://codereview.appspot.com/196058/diff/1/106#newcode48
src/node/radiotap-header.h:48: class RadiotapHeader : public Header
I feel that this class probably belongs to src/common instead.

http://codereview.appspot.com/196058/diff/1/106#newcode241
src/node/radiotap-header.h:241: enum {RADIOTAP_TSFT              = 0x00000001};
Is there a reason why this is not written as follows ?

enum
  {
    RADIOTAP_XXX    = ...,
    RADIOTAP_XXXX   = ...,
  };

[gjcarneiro, 2010-01-30 15:45:33]

http://codereview.appspot.com/196058/diff/1/21
File bindings/python/ns3modulescan.py (right):

http://codereview.appspot.com/196058/diff/1/21#newcode153
bindings/python/ns3modulescan.py:153: #if
pygccxml_definition.name.endswith('Helper'):
Do you really think users should be able to subclass the helper classes in
Python? Why?

[Tom Henderson, 2010-01-30 22:25:50]

I support the new architecture and API.  I mainly reviewed the new documentation
(nice job) and API, and sent some documentation nits to Craig privately.  My
lingering questions are 
1) the performance issue for wireless
2) whether backward compatible API is useful for at least one release cycle.

I think the most visible user impacts will be on people who use the Wifi, Csma,
and PointToPoint device tracing APIs; below are the pcap-related calls that I
think will be most impacted (ascii are similar):

-  CsmaHelper::EnablePcapAll ("csma-bridge", false);
+  csma.EnablePcapAll ("csma-bridge", false);

-  PointToPointHelper::EnablePcapAll ("simple-error-model");
+  p2p.EnablePcapAll ("simple-error-model");

-  wifiPhy.SetPcapFormat (YansWifiPhyHelper::PCAP_FORMAT_80211_RADIOTAP); 
+  wifiPhy.SetPcapDataLinkType (PcapHelper::DLT_IEEE802_11_RADIO); 

Can we add NS_DEPRECATED API support for the old static functions, at least for
the device helpers (the InternetStack helper seems harder to support, but I
would guess that it is not being heavily used since it is not in any examples,
so I don't care strongly about that one). 

I would also suggest to explicitly list (in unified diff) replacement API for
each removed method in the file CHANGES.html, not only the example provided. 

p.s. sixth.cc (implied by the tutorial documentation) didn't make it into the
repo

[craigdo, 2010-02-01 21:15:11]

http://codereview.appspot.com/196058/diff/1/21
File bindings/python/ns3modulescan.py (right):

http://codereview.appspot.com/196058/diff/1/21#newcode153
bindings/python/ns3modulescan.py:153: #if
pygccxml_definition.name.endswith('Helper'):
The issue is whether or not the YansWifiHelper can subclass WifiHelper and
WifiHelper can subclass trace *Helper.

Having an inheritance policy enforced by the last characters in a class name
seems scary.

http://codereview.appspot.com/196058/diff/1/77
File src/common/output-stream-object.h (right):

http://codereview.appspot.com/196058/diff/1/77#newcode72
src/common/output-stream-object.h:72: class OutputStreamObject : public Object
Deriving from Object means that a user can derive from something low level
enough but relatively familiar and documented without having to go into gory
details about something new.

http://codereview.appspot.com/196058/diff/1/78
File src/common/pcap-file-object.cc (right):

http://codereview.appspot.com/196058/diff/1/78#newcode75
src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
I want to do block I/O here since it is block I/O.  Similar things need to
happen in emu and tap at least.

http://codereview.appspot.com/196058/diff/1/79
File src/common/pcap-file-object.h (right):

http://codereview.appspot.com/196058/diff/1/79#newcode42
src/common/pcap-file-object.h:42: bool Open (std::string const &filename,
std::string const &mode);
Oops.

http://codereview.appspot.com/196058/diff/1/98
File src/helper/trace-helper.h (right):

http://codereview.appspot.com/196058/diff/1/98#newcode156
src/helper/trace-helper.h:156: Ptr<OutputStreamObject> CreateFileStream
(std::string filename, std::string filemode = "w");
I want a low-lvel thing that is agnostic about what kind of ascii is actually
written.  See sixth.cc for how this works.  I want a higher level thing to
actually write the ns-2-like ascii trace data.  I don't however, want to
hardcode class:DROP, class:ENQUEUE, etc.  Since this was all a bunch of
one-liners it seemed easier to rewrite than modify extensively.

http://codereview.appspot.com/196058/diff/1/102
File src/helper/yans-wifi-helper.h (right):

http://codereview.appspot.com/196058/diff/1/102#newcode242
src/helper/yans-wifi-helper.h:242: void SetPcapDataLinkType (uint32_t dlt);
In general I want data link types to be integers instead of enumerations so the
tracing system can be more easily extended.  Users will be using integers to
define all pcap types now, so it made more sense to use the same thing here
instead of forcing the user to look up an equivalent only useful in this class.

http://codereview.appspot.com/196058/diff/1/106
File src/node/radiotap-header.h (right):

http://codereview.appspot.com/196058/diff/1/106#newcode48
src/node/radiotap-header.h:48: class RadiotapHeader : public Header
I don't have any strong feelings about this.  It was originally in
src/devices/wifi and I moved it to src/node since that's where most of the rest
of the headers live.

I'm not sure why radiotap headers should be in src/common but ethernet headers
should be in src/node, for example.

I'm not sure why radiotap headers should be in common but ppp headers should be
in src/devices/point-to-point either.

?????

http://codereview.appspot.com/196058/diff/1/106#newcode241
src/node/radiotap-header.h:241: enum {RADIOTAP_TSFT              = 0x00000001};
This is the result of an emacs macro applied to #defines in the original code. 
I could go the one step further, but it didn't seem to buy much ...

[craigdo, 2010-02-01 21:18:27]

Will add doxygen to PcapFileObject

[Mathieu Lacage, 2010-02-02 09:11:30]

On Mon, Feb 1, 2010 at 10:15 PM, <craigdo@gmail.com> wrote:


> http://codereview.appspot.com/196058/diff/1/77#newcode72
> src/common/output-stream-object.h:72: class OutputStreamObject : public
> Object
> Deriving from Object means that a user can derive from something low
> level enough but relatively familiar and documented without having to go
> into gory details about something new.
>

nak: it makes no sense: you don't need all the goo from Object: this is
exactly why we have SimpleRefCount/RefCountBase (pick the one you prefer).

http://codereview.appspot.com/196058/diff/1/78
> File src/common/pcap-file-object.cc (right):
>
> http://codereview.appspot.com/196058/diff/1/78#newcode75
> src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
> I want to do block I/O here since it is block I/O.  Similar things need
> to happen in emu and tap at least.
>

I am sorry but I do not understand what you mean here:
1)  What do you call block io ?
2) This is not the tap/emu device so my comment is specifically not about
them. It is about this specific piece of code.


> http://codereview.appspot.com/196058/diff/1/98#newcode156
> src/helper/trace-helper.h:156: Ptr<OutputStreamObject> CreateFileStream
> (std::string filename, std::string filemode = "w");
> I want a low-lvel thing that is agnostic about what kind of ascii is
> actually written.  See sixth.cc for how this works.  I want a higher
> level thing to actually write the ns-2-like ascii trace data.  I don't
> however, want to hardcode class:DROP, class:ENQUEUE, etc.  Since this
> was all a bunch of one-liners it seemed easier to rewrite than modify
> extensively.
>

Hrm. I would be fine with the above if:
1) it (the distinction between low-level and high-level) was documented
somehow (at least, in output-stream-object.h)
2) the existing code using AsciiWriter had not been changed to not use it
anymore: it hardcodes instead the ns-2 ascii output everywhere now.

2), at least, needs to be fixed to revert to the old behavior because it
allows us to more easily control the ns-2 trace output.

Now, I have to confess that, independently from 2), 1) feels really a bit
overkill: I don't really understand why we can't just add the necessary
GetStream method to AsciiWriter instead of creating a new OutputStreamObject
class.


> http://codereview.appspot.com/196058/diff/1/102#newcode242
> src/helper/yans-wifi-helper.h:242: void SetPcapDataLinkType (uint32_t
> dlt);
> In general I want data link types to be integers instead of enumerations
> so the tracing system can be more easily extended.  Users will be using
> integers to define all pcap types now, so it made more sense to use the
> same thing here instead of forcing the user to look up an equivalent
> only useful in this class.
>

I read the documentation and I don't see how I am supposed to know what to
feed in this method. From the perspective of the user, this method is
replacing a method which was using an enum with well-defined
defintions/semantics so this replacement method is not an improvement: it's
a regression (again, from the perspective of a naive user who does not care
about the details).


> http://codereview.appspot.com/196058/diff/1/106#newcode48
> src/node/radiotap-header.h:48: class RadiotapHeader : public Header
> I don't have any strong feelings about this.  It was originally in
> src/devices/wifi and I moved it to src/node since that's where most of
> the rest of the headers live.
>
> I'm not sure why radiotap headers should be in src/common but ethernet
> headers should be in src/node, for example.
>
> I'm not sure why radiotap headers should be in common but ppp headers
> should be in src/devices/point-to-point either.
>
> ?????
>

Yes, I am not sure either about all this stuff. Maybe we should move all the
public headers in src/common. i.e., 'common' is a folder for low-level stuff
useful to write network models so, it seems to fit the bill.


> http://codereview.appspot.com/196058/diff/1/106#newcode241
> src/node/radiotap-header.h:241: enum {RADIOTAP_TSFT              =
> 0x00000001};
> This is the result of an emacs macro applied to #defines in the original
> code.  I could go the one step further, but it didn't seem to buy much
> ...
>

It's just really weird-looking code and we usually spend more time reading
it than writing it so, it probably makes sense to make it look not too weird
to avoid the inevitable question: why does this thing look like this ?

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[gjcarneiro, 2010-02-02 10:52:35]

On Tue, Feb 2, 2010 at 9:11 AM, Mathieu Lacage <mathieu.lacage@gmail.com>wrote:

>
>
> On Mon, Feb 1, 2010 at 10:15 PM, <craigdo@gmail.com> wrote:
>
>
>> http://codereview.appspot.com/196058/diff/1/77#newcode72
>> src/common/output-stream-object.h:72: class OutputStreamObject : public
>> Object
>> Deriving from Object means that a user can derive from something low
>> level enough but relatively familiar and documented without having to go
>> into gory details about something new.
>>
>
> nak: it makes no sense: you don't need all the goo from Object: this is
> exactly why we have SimpleRefCount/RefCountBase (pick the one you prefer).
>

I thought RefCountBase was deprecated:

 * \brief A deprecated way to get reference-counting powers

So, please make up your mind.

And it's a shame, I find SimpleRefCount scary, not so simple as the name
implies...

-- 
Gustavo J. A. M. Carneiro
INESC Porto, Telecommunications and Multimedia Unit
"The universe is always one step beyond logic." -- Frank Herbert

[craigdo1, 2010-02-02 20:07:27]

>>http://codereview.appspot.com/196058/diff/1/77#newcode72
>>src/common/output-stream-object.h:72: class OutputStreamObject : public
Object
>
>>Deriving from Object means that a user can derive from something low
>> level enough but relatively familiar and documented without having to go
>> into gory details about something new.
>
>nak: it makes no sense: you don't need all the goo from Object: this is
>exactly why we have SimpleRefCount/RefCountBase (pick the one you prefer).

I prefer to give the user something that is familiar and known at this
point.  I'm, talking new users here, not experienced maintainers or model
authors who know that there is a difference that exists and exactly what it
is.  I would prefer to give them something that, if it looks like a
Ptr<Object> it behaves like the other Ptr<Object> things they have been
exposed to and lets them do Object-like things.

The situation I want to avoid is if a user looks at Ptr<PcapFileObject> or
Ptr<OutputStreamObject>, notices the Ipv4 protocol trace stuff, reads about
aggregating protocols to nodes, and then decides it would be super-cool to
aggregate his trace file objects to carry them around.  The user could
design something and code to the bottom and then discover that even though
it looks like and Object, smells like an Object and walks like an Object, it
doesn't quack like an Object (doesn't support AggregateObject or GetObject).

I've been involved in discussions with new users and it is exactly at points
like this that they throw up their hands and walk away.  We will have saved
some minor complexity in an object, but made it work differently
(surprisingly differently to an inexperienced user).  To short-circuit
problems like those described above, I would have to mention somewhere in
the tutorial that these things are really not Objects.  They are reference
counted objects.  This is something I don't want to have to explain at that
level.

I think the gain in using something familiar to novice users, combined with
the additional flexibility offered, offsets the small cost of bringing in
some (potentially useful) goo.

>>http://codereview.appspot.com/196058/diff/1/78#newcode75
>>src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
>I want to do block I/O here since it is block I/O.  Similar things need
>to happen in emu and tap at least.
>
>I am sorry but I do not understand what you mean here:
>1)  What do you call block io ? 

Terminals are accessed as if they were unstructured streams of bytes  Disk
drives are accessed as if they were composed of  groupings of bytes called
blocks -- they have structure.  Hardware devices can be structured or
unstructured; and they are known as block or character devices,
respectively. 

Unstructured devices are those devices that do not support a block
structure.  An ascii trace file doesn't have any structure, for example, it
is just a stream of bytes.  A pcap trace file does have structure, it is a
collection of packets.  Just like a pcap file is written as a number of
packets, an emu or tap device writes a number of packets out a network
interface or a tap.

>2) This is not the tap/emu device so my comment is specifically not about
them. It is about this specific piece of code. 

I want the code to perform well in all cases and not have block I/O be
something on the side.  Exactly the same low-level code needs to run in all
three cases to get the needed block of data out of the Packet.  i.e., there
is no std::sendto (std::ostream &) function.  The question of the usefulness
of the concept of block I/O goes to all three cases and I do not believe it
is any kind of bug to think of block I/O in pcap in terms of block I/O.
That is what it is.
 
>>http://codereview.appspot.com/196058/diff/1/98#newcode156
>>src/helper/trace-helper.h:156: Ptr<OutputStreamObject> CreateFileStream
>>(std::string filename, std::string filemode = "w");
>I want a low-lvel thing that is agnostic about what kind of ascii is
>actually written.  See sixth.cc for how this works.  I want a higher
>level thing to actually write the ns-2-like ascii trace data.  I don't
>however, want to hardcode class:DROP, class:ENQUEUE, etc.  Since this
>was all a bunch of one-liners it seemed easier to rewrite than modify
>extensively.

>Hrm. I would be fine with the above if:
>1) it (the distinction between low-level and high-level) was documented
somehow (at least, in output-stream-object.h)

I thought the distinction would be clear from the names:  output stream
object vs. ascii trace helper.  One makes it easy to write generic streams
of bytes.  The other helps you make ascii trace files.  I can add more words
to the Doxygen if you  think it is worthwhile.

>2) the existing code using AsciiWriter had not been changed to not 
>use it anymore: it hardcodes instead the ns-2 ascii output everywhere now.

This isn't true.  Look at csma-helper.cc and notice that nowhere is the ns-2
ascii output hardcoded.  Look at point-to-point-helper.cc and notice the
same thing.  Look at emu-helper.  Same thing.

This is because the ns-2-like ascii output is done in the AsciiTraceHelper.
The only difference is that the "+", "-", and "d" is in plain sight there
(in the default trace sinks) instead of coded as ENQUEUE, DEQUEUE, DROP and
passed to another lower-level routine which simply does a switch on the
encoded value back to "+", "-", and "d".

Trace sinks that correspond to the default sink signature do use a common
piece of code to define what goes into the ascii trace file.  The one place
that this is not the case is in yans-wifi-helper.cc which is hardly
everywhere.  In this case, the trace sinks have additional parameters: a
WifiMode, a WifiPreamble and a txLevel.  From this perspective, at least, it
seems useful to allow a model author to specify a new trace entry and print
whatever he or she wants for a new device if needed.

It turns out that that is exactly what was done for the yans-wifi-helper and
is a great example of why I want to do it the way I did.  The "standard
model" on all of the other devices is to have a Queue on the device through
which all packets flow.  Traces on the Queue give the "+", "-" and "d"
events.  The yans wifi helper decided to do it completely differently.
There is no single queue to flow through, so new trace events "r" and "w"
were defined for that helper.  In order to work with the AsciiWriter, it was
required to define new types RX and TX and plumb the change down into the
AsciiWriter and expand the switch statement to accommodate the new cases.
This required changing the system core, which I want to avoid.

In the new environment, you can write a new device helper, hook your traces
and print your new identifiers ("r" or "t") directly without changing any
lower level code.

If you have a new device with new, non-standard trace sources, you can
always ignore your new information and use a pre-defined default sink to
write your data.  For example, you could just call,

  AsciiTraceHelper::DefaultEnqueueSinkWithContext (stream, context, packet);

instead of writing to the stream directly.  These are publicly defined
static functions that are there just for that purpose.  If you want to do
something completely different (like the Cwnd example in sixth.cc) then you
just use the OutputStreamObject to carry around your stream and write
whatever you want to its stream (letting it deal with stream lifetime and
copy constructor issues).

I think this is a better solution, not a problem.

> 2), at least, needs to be fixed to revert to the old behavior because 
> it allows us to more easily control the ns-2 trace output.

I believe it is already "fixed".

>Now, I have to confess that, independently from 2), 1) feels really a bit 
> overkill: I don't really understand why we can't just add the necessary 
> GetStream method to AsciiWriter instead of creating a new 
> OutputStreamObject class.

I think that if you fix AsciiWriter to address the problems with tracing
that have been addressed, you will find two classes that behave like the
OutputStreamObject and AsciiTraceHelper -- you will just end up with
OutputStreamObject and AsciiTraceHelper.

Think about sixth.cc, all of the device-related helpers except
yans-wifi-helper and then yans-wifi-helper and see how the new approach
accommodates them all.  Then see how a single AsciiWriter with hard-coded
types gets in the way.  Then think about how you would fix it all.

Like I said, I think this is a better solution, not a problem.
 
>>http://codereview.appspot.com/196058/diff/1/102#newcode242
>>src/helper/yans-wifi-helper.h:242: void SetPcapDataLinkType (uint32_t
dlt);
>>In general I want data link types to be integers instead of enumerations
>>so the tracing system can be more easily extended.  Users will be using
>>integers to define all pcap types now, so it made more sense to use the
>>same thing here instead of forcing the user to look up an equivalent
>>only useful in this class.
>
>I read the documentation and I don't see how I am supposed to know what to
feed in this method. From the perspective of the >user, this method is
replacing a method which was using an enum with well-defined
defintions/semantics so this replacement >method is not an improvement: it's
a regression (again, from the perspective of a naive user who does not care
about the >details). 

This is a boundary case that goes to the underlying definition of a pcap
file.  I chose overall consistency for the API instead of making a separate
enum that just represents a DLT in this one place.

When you work with pcap files, you must somehow choose a DLT.  That tells
pcap readers (tcpdump, wireshark) what format the individual packets will
take.  The DLT is not a property of the pcap format, it is associated with
the content of the format.  Therefore, you don't want to make an enumeration
of all of the valid pcap types in the system.  This would make it difficult
for a user to save pcap files in with a new DLT.  This means that DLTs need
to be integers, and not an enumeration.

So, now we have a place in the system (yans-wifi-helper) which supports
three specific data link types in the pcap file that it writes.  The user is
already thinking in terms of the data link type of the pcap file he or she
wants.  The question is, do you allow the user to tell the yans-wifi-helper
what he or she wants in terms of DLTs; or do you force that user to
translate the desired DLT into another yans-wifi-helper-specific
enumeration, which the helper will then translate back into the DLT the user
ultimately wanted in the pcap file in the first place?

The tradeoff is a compile-time error with an incorrect enumeration, versus a
run-time error when the helper decides it can't support the DLT.  It seemed
a bit obtuse to force a user to convert from DLT to enum and then have the
helper convert back to DLT when creating the file.  I chose the integer DLT.

I think this is the best approach from a global API and conceptual model
perspective; but I do understand your concern and took it into account when
designing the API.

>>http://codereview.appspot.com/196058/diff/1/106#newcode48
>>src/node/radiotap-header.h:48: class RadiotapHeader : public Header
>>I don't have any strong feelings about this.  It was originally in
>>src/devices/wifi and I moved it to src/node since that's where most of
>>the rest of the headers live.
>>
>>I'm not sure why radiotap headers should be in src/common but ethernet
>>headers should be in src/node, for example.
>>
>>I'm not sure why radiotap headers should be in common but ppp headers
>>should be in src/devices/point-to-point either.
>>
>>?????
>
>Yes, I am not sure either about all this stuff. Maybe we should move 
> all the public headers in src/common. i.e., 'common' is a folder for 
> low-level stuff useful to write network models so, it seems to fit the
bill. 
 
That makes sense.  Do you want to file a bug about randomness in where we
put our headers?

>>http://codereview.appspot.com/196058/diff/1/106#newcode241
>>src/node/radiotap-header.h:241: enum {RADIOTAP_TSFT              =
>>0x00000001};
>>This is the result of an emacs macro applied to #defines in the original
>>code.  I could go the one step further, but it didn't seem to buy much
>>...
>
>It's just really weird-looking code and we usually spend more time 
>reading it than writing it so, it probably makes sense to make it 
>look not too weird to avoid the inevitable question: why does this
>thing look like this ?

Okay.  I buy that argument.

[Tom Henderson, 2010-02-03 15:05:10]

FWIW, here are my opinions on what seem to be the contentious issues.

http://codereview.appspot.com/196058/diff/1/77
File src/common/output-stream-object.h (right):

http://codereview.appspot.com/196058/diff/1/77#newcode72
src/common/output-stream-object.h:72: class OutputStreamObject : public Object
On 2010/02/01 21:15:11, craigdo wrote:
> Deriving from Object means that a user can derive from something low level
> enough but relatively familiar and documented without having to go into gory
> details about something new.
> 
> 

Although I do not care whether this class ends up deriving from Object or not, I
would have chosen reference counting base class for the following reasons:

1) I don't think the non-Object base class is a foreign concept (we use it
elsewhere such as Ipv{4|6}Route, and AsciiWriter, and should use where
applicable).  I think there may be legacy uses in the system that predate the
introduction of the alternative base classes (such as PcapWriter).  It is
clearly documented, if you need attributes and aggregation, use Object; if you
don't need these features, we provide other base classes.

2) I didn't find the possible use case of aggregating stream objects to a node
(note that we can only aggregate one instance) that likely.  I think that if
such a need arises, one could create a StreamContainerObject : public Object and
aggregate that.  


Regarding the class as a whole, I think it is useful to have just to encapsulate
references to stream as exemplified in sixth.cc.  Rather than reuse AsciiWriter,
I thought that perhaps AsciiWriter could inherit from this class, until I
learned this morning that AsciiWriter is slated for deletion.

http://codereview.appspot.com/196058/diff/1/78
File src/common/pcap-file-object.cc (right):

http://codereview.appspot.com/196058/diff/1/78#newcode75
src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
On 2010/02/01 21:15:11, craigdo wrote:
> I want to do block I/O here since it is block I/O.  Similar things need to
> happen in emu and tap at least.
> 

I don't have a strong opinion here since the test cases show 1% improvement in
avoiding the allocation/copy.  I don't see why we wouldn't just put in the
faster variant and leave a comment that "this architecturally should use block
I/O but the below is optimized to avoid extra allocation/copy".

http://codereview.appspot.com/196058/diff/1/98
File src/helper/trace-helper.h (right):

http://codereview.appspot.com/196058/diff/1/98#newcode57
src/helper/trace-helper.h:57: enum {DLT_IEEE802_11_RADIO = 127};
I favor using the DLT_ constants as introduced by Craig, although I will be fine
with either.

http://codereview.appspot.com/196058/diff/1/98#newcode940
src/helper/trace-helper.h:940: class TraceHelperForProtocol : public
PcapAndAsciiHelperForIpv4AndPcapHelperForIpv6
I don't have a strong opinion about mixins vs lumping all of this together
(seems like both could be made to work adequately), other than I agree that the
above is pretty horrific and should be fixed somehow.

[gjcarneiro, 2010-02-03 15:12:31]

I like the introduction of an OutputStreamObject class. It fixes an old
Python bindings bug (Bug 155 - "std::ostream & os" parameters not Python
friendly).

On Wed, Feb 3, 2010 at 3:05 PM, <tomh.org@gmail.com> wrote:

> FWIW, here are my opinions on what seem to be the contentious issues.
>
>
>
> http://codereview.appspot.com/196058/diff/1/77
> File src/common/output-stream-object.h (right):
>
>
> http://codereview.appspot.com/196058/diff/1/77#newcode72
> src/common/output-stream-object.h:72: class OutputStreamObject : public
> Object
> On 2010/02/01 21:15:11, craigdo wrote:
>
>> Deriving from Object means that a user can derive from something low
>>
> level
>
>> enough but relatively familiar and documented without having to go
>>
> into gory
>
>> details about something new.
>>
>
>
>
> Although I do not care whether this class ends up deriving from Object
> or not, I would have chosen reference counting base class for the
> following reasons:
>
> 1) I don't think the non-Object base class is a foreign concept (we use
> it elsewhere such as Ipv{4|6}Route, and AsciiWriter, and should use
> where applicable).  I think there may be legacy uses in the system that
> predate the introduction of the alternative base classes (such as
> PcapWriter).  It is clearly documented, if you need attributes and
> aggregation, use Object; if you don't need these features, we provide
> other base classes.
>
> 2) I didn't find the possible use case of aggregating stream objects to
> a node (note that we can only aggregate one instance) that likely.  I
> think that if such a need arises, one could create a
> StreamContainerObject : public Object and aggregate that.
>
>
> Regarding the class as a whole, I think it is useful to have just to
> encapsulate references to stream as exemplified in sixth.cc.  Rather
> than reuse AsciiWriter, I thought that perhaps AsciiWriter could inherit
> from this class, until I learned this morning that AsciiWriter is slated
> for deletion.
>
>
> http://codereview.appspot.com/196058/diff/1/78
> File src/common/pcap-file-object.cc (right):
>
>
> http://codereview.appspot.com/196058/diff/1/78#newcode75
> src/common/pcap-file-object.cc:75: p->CopyData (buffer, bufferSize);
> On 2010/02/01 21:15:11, craigdo wrote:
>
>> I want to do block I/O here since it is block I/O.  Similar things
>>
> need to
>
>> happen in emu and tap at least.
>>
>
>
> I don't have a strong opinion here since the test cases show 1%
> improvement in avoiding the allocation/copy.  I don't see why we
> wouldn't just put in the faster variant and leave a comment that "this
> architecturally should use block I/O but the below is optimized to avoid
> extra allocation/copy".
>
> http://codereview.appspot.com/196058/diff/1/98
> File src/helper/trace-helper.h (right):
>
> http://codereview.appspot.com/196058/diff/1/98#newcode57
> src/helper/trace-helper.h:57: enum {DLT_IEEE802_11_RADIO = 127};
> I favor using the DLT_ constants as introduced by Craig, although I will
> be fine with either.
>
> http://codereview.appspot.com/196058/diff/1/98#newcode940
> src/helper/trace-helper.h:940: class TraceHelperForProtocol : public
> PcapAndAsciiHelperForIpv4AndPcapHelperForIpv6
> I don't have a strong opinion about mixins vs lumping all of this
> together (seems like both could be made to work adequately), other than
> I agree that the above is pretty horrific and should be fixed somehow.
>
> http://codereview.appspot.com/196058/show
>



-- 
Gustavo J. A. M. Carneiro
INESC Porto, Telecommunications and Multimedia Unit
"The universe is always one step beyond logic." -- Frank Herbert