Bug 491

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 attachment 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 attachment 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 attachment 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]
> 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