After discussing this with tom, the solution would be:

1) make InternetStackHelper derive from ObjectBase
2) call   ObjectBase::ConstructSelf (AttributeList ()); from
InternetStackHelper::InternetStackHelper
3) Implement InternetStackHelper::GetTypeId and GetInstanceTypeId
4) Add a "CalcChecksum" attribute to InternetStackHelper
5) use the value of the CalcChecksum attribute from Install during object
instanciation.

I got bit by this again today.  V4Ping creates an icmp header outside of the
Icmpv4L4Protocol where there is a calcChecksum.  Checksums in V4Ping aren't
turned on when you turn on checksums in icmp.

It's time to fix this.

I don't think that adding a calChecksum to InternetStackHelper is the right
thing to do.  There is no law that says that a script must ever instantiate an
internet stack helper.  We need something that turns on checksums irrespective
of what level script one is using.  

Like I said, I think there should be a global enable-checksums option that
trumps the individual checksum attributes and just turns on all checksums in
the system.  All checksum enabling code should respect this global value.

I would like to fix this tomorrow to get emu-ping working.  How about adding a
GlobalValue in the internet stack module?

(In reply to comment #2)
> I got bit by this again today.  V4Ping creates an icmp header outside of the
> Icmpv4L4Protocol where there is a calcChecksum.  Checksums in V4Ping aren't
> turned on when you turn on checksums in icmp.
> 
> It's time to fix this.
> 
> I don't think that adding a calChecksum to InternetStackHelper is the right
> thing to do.  There is no law that says that a script must ever instantiate an
> internet stack helper.  We need something that turns on checksums irrespective
> of what level script one is using.  
> 
> Like I said, I think there should be a global enable-checksums option that
> trumps the individual checksum attributes and just turns on all checksums in
> the system.  All checksum enabling code should respect this global value.
> 
> I would like to fix this tomorrow to get emu-ping working.  How about adding a
> GlobalValue in the internet stack module?
> 


I would like to see this feature so that there is a single-line or almost
single-line way to control this behavior.

I do not have a strong opinion on whether it is a global value or a helper; I
see your argument but also could argue that global value is not the typical way
we solve these types of problems in our system, and that it should be trivial
to instantiate an Internet stack helper (or simply make it a static class
method) to get access to this capability.  

(In reply to comment #2)

> I don't think that adding a calChecksum to InternetStackHelper is the right
> thing to do.  There is no law that says that a script must ever instantiate an
> internet stack helper.  We need something that turns on checksums irrespective
> of what level script one is using.  
> 
> Like I said, I think there should be a global enable-checksums option that
> trumps the individual checksum attributes and just turns on all checksums in
> the system.  All checksum enabling code should respect this global value.
> 
> I would like to fix this tomorrow to get emu-ping working.  How about adding a
> GlobalValue in the internet stack module?

Per your above argument that we should not have an option in
InternetStackHelper, the global value should not be in internet-stack.

Available options:
1) make this an attribute of the Node object
2) make this a global value defined in src/node/node.cc
3) define this global value somewhere else.

Created an attachment (id=431) [details]
Add a system object to provide a place for common configuration attributes

Proposed system object

I couldn't bring myself to just add another random GlovalValue so I added a
"System Object" that provides an easily findable place to put common
configuration items such as a global "CalcChecksum" and others such as the
simulator implementation type, etc.

Patch attached.

Thoughts?

(In reply to comment #6)
> I couldn't bring myself to just add another random GlovalValue so I added a
> "System Object" that provides an easily findable place to put common
> configuration items such as a global "CalcChecksum" and others such as the
> simulator implementation type, etc.
> 
> Patch attached.
> 
> Thoughts?
> 

Did you forget to hg add the new files?  I don't see them in the patch.

Created an attachment (id=432) [details]
Complete patch to show how a system object would work

Forgot to hg add the two new (very small) files.

You now have a singleton object with global configuration information.  From
examples/emu-ping.cc turning on checksums is a one-liner:

  System ()->SetAttribute ("CalcChecksum", BooleanValue (true));

The protocols do something like,

  BooleanValue b;
  System ()->GetAttribute ("CalcChecksum", b);

  if(m_calcChecksum || b.Get ())
    {
      ...

(In reply to comment #9)
> Forgot to hg add the two new (very small) files.
> 
> You now have a singleton object with global configuration information.  From
> examples/emu-ping.cc turning on checksums is a one-liner:
> 
>   System ()->SetAttribute ("CalcChecksum", BooleanValue (true));
> 
> The protocols do something like,
> 
>   BooleanValue b;
>   System ()->GetAttribute ("CalcChecksum", b);
> 
>   if(m_calcChecksum || b.Get ())
>     {
>       ...
> 

I would be fine with this solution, although it does not use one attribute or
global value to set the others, the net user-visible behavior is the same.

(In reply to comment #6)
> I couldn't bring myself to just add another random GlovalValue so I added a
> "System Object" that provides an easily findable place to put common
> configuration items such as a global "CalcChecksum" and others such as the
> simulator implementation type, etc.
> 
> Patch attached.
> 
> Thoughts?

Well, that is pretty weird: I thought that this was the purpose of
GlobalValues. If you really want to go down that path/patch, then, removing the
GlobalValue code would be the way to go.

(In reply to comment #9)
> Forgot to hg add the two new (very small) files.
> 
> You now have a singleton object with global configuration information.  From
> examples/emu-ping.cc turning on checksums is a one-liner:
> 
>   System ()->SetAttribute ("CalcChecksum", BooleanValue (true));
> 
> The protocols do something like,
> 
>   BooleanValue b;
>   System ()->GetAttribute ("CalcChecksum", b);
> 
>   if(m_calcChecksum || b.Get ())
>     {
>       ...

low-level details:
  - why do you bother with keeping the m_calcChecksum attribute ? It seems to
just increase the number of options for no good reason.

  - the SystemObject should be registered in the global config namespace as
/System with Config::RegisterRootNamespaceObject.

high-level comments:
While I see why it sort of makes sense to do this for this specific attribute
because it has to be used in multiple locations, most other uses of GlobalValue
should not be converted to that API because it's not nice to have to define
them all in a single global source file and that is why we have been using
GlobalValues: to distribute the definition of each global attribute to where
it's really used in the code.

To summarize, unless there are other clear usecases of needing an attribute
which is readonly shared among multiple source files (which is not the case for
all other GlobalValue instances), I do not support adding this new generic API
and I would support instead coming up with an adhoc solution for that specific
usecase of a checksum.

i.e., I would support instead what I suggested earlier, that is, a static or
non-static Node::ChecksumEnabled method implemented using either a GlobalValue
or a node attribute. (The static/GlobalValue variant feels somewhat better).
(feel free to come up with a better name).

> Well, that is pretty weird: I thought that this was the purpose of
> GlobalValues. If you really want to go down that path/patch, then, 
> removing the GlobalValue code would be the way to go.

That was the idea.

Right now the global values are kind of scattered around the system and are
hard to find.  The enable checksum value spans node and internet stack and I
can think of reasons to put in both places.

In either case, you end up with a global variable that lifes in one place and
affects code in other places.  It seemed like a good idea to have a place to
put these things that worked with the rest of the system.  For example,
system-wide attributes would appear in the list of all attributes for easier
documentation.  Unless I'm mistaken, I don't think there is a list of all
global values anywhere in the documentation.

So, yes, the purpose of global values is to be global values.  I'm proposing
that we consider a set of system configuration attributes to take their place. 
It seems more organized and easier to deal with.

(In reply to comment #13)
> > Well, that is pretty weird: I thought that this was the purpose of
> > GlobalValues. If you really want to go down that path/patch, then, 
> > removing the GlobalValue code would be the way to go.
> 
> That was the idea.
> 
> Right now the global values are kind of scattered around the system and are
> hard to find.  The enable checksum value spans node and internet stack and I
> can think of reasons to put in both places.
> 
> In either case, you end up with a global variable that lifes in one place and
> affects code in other places.  It seemed like a good idea to have a place to
> put these things that worked with the rest of the system.  For example,
> system-wide attributes would appear in the list of all attributes for easier
> documentation.  Unless I'm mistaken, I don't think there is a list of all
> global values anywhere in the documentation.

There is none but it should be fairly trivial to extend
print-introspected-doxygen.cc to generate it: all the API to do so is there.

To summarize, I have a fundamental dislike of global variables.  I think it
would be cleaner to have a system configuration object.

There are now, I think, seven candidates for a system object:

  RngSeed
  RngRun
  SimulatorImplementationType
  SchedulerType
  TimeStepPrecision
  CalcChecksum

That could be used like,

  Config::Set ("/System/RngSeed", IntegerValue (1234));

or

  System ()->SetAttribute ("RngSeed", IntegerValue (1234));

instead of

  Config::SetGlobal ("RngSeed", IntegerValue (1234"));

The difference in the code is that the global variables are not "scattered
around the system" and can be used across multiple files.  I think that
conceptually, this is much better.  One does not have some global variable
defines somewhere deep down in the system.  You don't have to hunt around for
system configuration items, they are in one place.  The danger seems to be that
system configuration items grow to number hundreds or thousands.  I think that
the introduction of Attributes minimizes that risk.  

The end result is that models have their attributes and the system has its
attributes that are gotten to and modified in the same way.  Seems very
consistent and easy to deal with.

Anyway, it seems like an obviously good thing to me. but I'm not going to press
this endlessly.

Given where we are, I do not have a strong opinion on this; I see the API
consistency of Craig's proposal as a plus, but on the other hand, we have
GlobalValue already implemented and used.  

Mathieu, if you decide to keep GlobalValue, I suggest to just file a new bug on
adding introspection for these, and to put the GlobalValue for this one in
Node, I guess.

(In reply to comment #16)
> Given where we are, I do not have a strong opinion on this; I see the API
> consistency of Craig's proposal as a plus, but on the other hand, we have
> GlobalValue already implemented and used.  
> 
> Mathieu, if you decide to keep GlobalValue, I suggest to just file a new bug on
> adding introspection for these, and to put the GlobalValue for this one in
> Node, I guess.
> 

Should we add a GlobalValue in the node class, and try to close this out?

Created an attachment (id=462) [details]
EnableChecksum global value

Do you want to also remove the individual attributes used by individual
protocol classes ?

(In reply to comment #18)
> Created an attachment (id=462) [details] [details]
> EnableChecksum global value
> 
> Do you want to also remove the individual attributes used by individual
> protocol classes ?
> 

Yes, unless someone thinks of a use case where these should be selectively
enabled and disabled.

changeset 31e9053749bb