Bug 2111

patch is pending in the codereview issue

This is a bump of this issue:

https://codereview.appspot.com/214420043/

do we want to modify this header or add another header?

I stand my point: a new header is a better choice (backward-compatibility wise)

Created attachment 2367 [details]
new header

The new header, slightly modified with respect to the original ones proposed by Matt.

you might want to change "Author: Mathieu Lacage <mathieu.lacage@sophia.inria.fr>" ?

As it stands the header is not used in ns3: if it results in adding deadcode, thus it should not be added to ns3. My initial stance was to add a feature (while keeping backwards compatibility).

(In reply to Matthieu Coudron from comment #5)
> you might want to change "Author: Mathieu Lacage
> <mathieu.lacage@sophia.inria.fr>" ?
> 
> As it stands the header is not used in ns3: if it results in adding
> deadcode, thus it should not be added to ns3. My initial stance was to add a
> feature (while keeping backwards compatibility).

I'll change the author. About adding a functionality and preserving backward compatibility, they are two conflicting goals.

Adding functionality means adding bytes, and this changes the actual behaviour.
Practically speaking, the applications that are using SeqTs header will change their minimum payload size from 12 bytes to 20 bytes.

About the dead code, if this header is included we could add a simple modification to UdpEcho apps and use it there (showing how to calculate a round trip delay at application layer). Also this is a behaviour change, but I'd rather do this. People could have used SeqTs in their work and rely on the fact that it's 12 bytes long.

(In reply to Tommaso Pecorella from comment #6)
> (In reply to Matthieu Coudron from comment #5)
> > you might want to change "Author: Mathieu Lacage
> > <mathieu.lacage@sophia.inria.fr>" ?
> > 
> > As it stands the header is not used in ns3: if it results in adding
> > deadcode, thus it should not be added to ns3. My initial stance was to add a
> > feature (while keeping backwards compatibility).
> 
> I'll change the author. About adding a functionality and preserving backward
> compatibility, they are two conflicting goals.
> 
> Adding functionality means adding bytes, and this changes the actual
> behaviour.
> Practically speaking, the applications that are using SeqTs header will
> change their minimum payload size from 12 bytes to 20 bytes.
> 
> About the dead code, if this header is included we could add a simple
> modification to UdpEcho apps and use it there (showing how to calculate a
> round trip delay at application layer). Also this is a behaviour change, but
> I'd rather do this. People could have used SeqTs in their work and rely on
> the fact that it's 12 bytes long.

I lean towards adding separate header also; we could add an example on how it is used.

I have two questions:

1) what about aligning field names and class name more with TCP Timestamps?

http://freesoft.org/CIE/RFC/1323/8.htm

so, for instance, we would have a "TsValue" and a "TsEchoReply" field instead of "Transmit" and "Receive" value.

We could also perhaps change the name to "SeqTsEchoHeader" which IMO may be clearer to users than "SeqTxRxTsHeader" (i.e. we can document to use "SeqTsHeader" if you do not care about adding the Echo field).

2) I don't like the asymmetry of setting the Ts with uint64_t but getting with class Time.  Why not this instead (on top of the name change I suggest above)?

-  void SetRxTs (uint64_t ts);
-  void SetTxTs (uint64_t ts);
+ void SetRxTs (Time ts);
+ void SetTxTs (Time ts);

(In reply to Tom Henderson from comment #7)
> ... 
> I lean towards adding separate header also; we could add an example on how
> it is used.
> 
> I have two questions:
> 
> 1) what about aligning field names and class name more with TCP Timestamps?
> 
> http://freesoft.org/CIE/RFC/1323/8.htm
> 
> so, for instance, we would have a "TsValue" and a "TsEchoReply" field
> instead of "Transmit" and "Receive" value.
> 
> We could also perhaps change the name to "SeqTsEchoHeader" which IMO may be
> clearer to users than "SeqTxRxTsHeader" (i.e. we can document to use
> "SeqTsHeader" if you do not care about adding the Echo field).

Fine by me. I wasn't particularly happy with the name, but I couldn't think to something better. This is better.


> 2) I don't like the asymmetry of setting the Ts with uint64_t but getting
> with class Time.  Why not this instead (on top of the name change I suggest
> above)?
> 
> -  void SetRxTs (uint64_t ts);
> -  void SetTxTs (uint64_t ts);
> + void SetRxTs (Time ts);
> + void SetTxTs (Time ts);

Mmmm... right. But this raises one problem: the time resolution.
Since we're using a completely new header, we can go bold and use a full 128 bit serialization, what do you think ?

> > 2) I don't like the asymmetry of setting the Ts with uint64_t but getting
> > with class Time.  Why not this instead (on top of the name change I suggest
> > above)?
> > 
> > -  void SetRxTs (uint64_t ts);
> > -  void SetTxTs (uint64_t ts);
> > + void SetRxTs (Time ts);
> > + void SetTxTs (Time ts);
> 
> Mmmm... right. But this raises one problem: the time resolution.
> Since we're using a completely new header, we can go bold and use a full 128
> bit serialization, what do you think ?

I'm not following; the time value is an underlying int64_t.  Why must resolution be carried?

Created attachment 2368 [details]
new headers

(In reply to Tom Henderson from comment #9)
> > > 2) I don't like the asymmetry of setting the Ts with uint64_t but getting
> > > with class Time.  Why not this instead (on top of the name change I suggest
> > > above)?
> > > 
> > > -  void SetRxTs (uint64_t ts);
> > > -  void SetTxTs (uint64_t ts);
> > > + void SetRxTs (Time ts);
> > > + void SetTxTs (Time ts);
> > 
> > Mmmm... right. But this raises one problem: the time resolution.
> > Since we're using a completely new header, we can go bold and use a full 128
> > bit serialization, what do you think ?
> 
> I'm not following; the time value is an underlying int64_t.  Why must
> resolution be carried?

Right, I was totally sure that the internal Time representation was 128 bits. Scratch the 128 bits idea, here's the new patch.
It's drycoded, so I could have missed something.