RE: final follow-up review of last-call

Hi all,

I had an offline discussion with Anders who provided a patch for the
issues discussed below.

So, for the 3 remaining issues, we have fixed them according to Anders
suggestions, i.e.
- Default for checksum_used
- Default static encodings for some of the global control fields,
  it turned out there wasn't a need for that many as we expected
initially
  therefore we didn't need to add a global DEFAULT section
- Fixed the df-issue as described below.

Expect a final submission of this draft tomorrow since we have now
ironed
out the final review issues reported.

/Kristofer

Anders Edqvist <mailto:[email protected]> wrote on den 28
januari 2008 10:14 :

> Kristofer, Ghyslain
> 
> I ACK your NACKs to the NACKs we responded with to your NACKs on our
> first review unless they are in this email  ;-)
> 
>>>>> 		All global control fields should have static encoding by
>>>>> 		default. This to ensure that they can keep their
previous
>>>>> values between packets.
>>>>> 
>>>>> 
>>>>> 
>>>>> 	NACK, all the global control fields (except MSN) are such that:
>>>>> 	a) They are "user-selected" for each input packet
>>>>> 	b) They are calculated from another field
>>>>> 
>>>>> 	So there is no case that I can see where a default
>>>>> would have any
>>>>> 	impact in the spec
>>>>> 
>>>>> 
>>>>> As an example: At the decompressor the reorder_ratio_value
>>>>> parameter for the baseheader is undefined, the control field
>>>>> is also undefined and thus msn_lsb fails for some compressed
>>>>> header formats (not co_repair). In our understanding you
>>>>> don't remember values between packets unless they are static
>>>>> encoded. 
>>>>> 
>>> 
>>> Not sure this is necessary, but we can add a DEFAULT
>>> section on the global scope. However, this is not supported by the
>>> FN in RFC 4997, so we would have to add a definition that explains
>>> this additional FN feature. Doing anything else would make the FN
>>> code become incredibly ugly, and we think that this is unnecessary.
>>> 
> I strongly feel this is necessary. Personally I opt for the uglier
> approach and specifying them in the encoding methods for chains
> and/or baseheaders instead. 
> 
>>>>> 
>>>>> 		There is a problem with how DF is handled in
>>>>> IPv6: Since DF has
>>>>> 		ULENGTH==0 for ipv6 but methods like dont_fragment and
>>>>> 		profile_1_7_flags_enc has a DF with ULENGTH==1
>>>>> in its UNCOMPRESSED
>>>>> 		format the binding will not be awesome. The
>>>>> concatenated list of
>>>>> 		flags will simply not match as intended to the
>>>>> bitpattern in the
>>>>> 		method. This problem affects all profiles and several
encoding
>>>>> methods. 
>>>>> 
>>>>> 
>>>>> 
>>>>> 	Changed dont_fragment to have the following
>>>>> UNCOMPRESSED section.
>>>>> 
>>>>> 	  UNCOMPRESSED {
>>>>> 	    df [ 0, 1 ];
>>>>> 	  }
>>>>> 
>>>>> 	Should fix the entire issue.
>>>>> 
>>>>> 
>>>>> The same change needs to be done to "callers" of
>>>>> dont_fragment. For instance "profile_1_7_flags1_enc" still
>>>>> returns DF as 1 bit... same applies for
>>>>> profile_2_3_4_flags_enc and profile_8_flags_enc.
>>> 
>>> NACK. These encodings only "return" the CLENGTH of 1 bit, which is
>>> what we want, it never re-binds ULENGTH to 1 bit for IPv6.
>>> No change.
>>> 
> Nothing is rebound, it just fails. The UNCOMPRESSED format of an
> encoding method is the UVALUE and ULENGTH. The concatenated list of
> fields will not match the UNCOMPRESSED format of the encoding method.
> 
> I suggest changes as the following
> 
> profile_1_7_flags1_enc(flag, ip_version)
> {
>   UNCOMPRESSED {
>     ip_outer_indicator  [ 1 ];
>     ttl_hopl_indicator  [ 1 ];
>     tos_tc_indicator    [ 1 ];
>     df                  [ 0, 1 ]; // only change here!
>     ip_id_behavior      [ 2 ];
>     reorder_ratio       [ 2 ];
>   }
> 
>   COMPRESSED not_present {
>     ENFORCE(flag == 0);
>     ENFORCE(ip_outer_indicator.CVALUE == 0);
>     ENFORCE(ttl_hopl_indicator.CVALUE == 0);
>     ENFORCE(tos_tc_indicator.CVALUE == 0);
>     df                   =:= static;
>     ip_id_behavior       =:= static;
>     reorder_ratio        =:= static;
>   }
> 
>   COMPRESSED present {
>     ENFORCE(flag == 1);
>     ip_outer_indicator  =:= irregular(1)                [ 1 ];
>     ttl_hopl_indicator  =:= irregular(1)                [ 1 ];
>     tos_tc_indicator    =:= irregular(1)                [ 1 ];
>     df                  =:= dont_fragment(ip_version)   [ 1 ];
>     ip_id_behavior      =:= irregular(2)                [ 2 ];
>     reorder_ratio       =:= irregular(2)                [ 2 ];   }
> }
> 
> Apply the same for profile_2_3_4_flags_enc(flag, ip_version) and
> profile_8_flags_enc(flag, ip_version)
> 
>>>>> 
>>>>> 
>>>>> 		udp()
>>>>> 		The control field checksum_used should have
>>>>> static encoding
>>>>> 		by default.
>>>>> 
>>>>> 
>>>>> 
>>>>> 	NACK, this field is always bound so a default has no meaning.
>>>>> 
>>>>> 
>>>>> It is not bound for the decompressor for CO headers (except
>>>>> co_repair) when parsing the irregular chain.
>>> 
>>> NACK. The irregular chain binds the UVALUE with the ENFORCE, and the
>>> ULENGTH gets bound in the CONTROL section, so everything is bound
>>> by the current code. No change.
>>> 
> Yes you are right that it is always bound, however the
> decompressor can
> not choose between udp_zero_checksum_irregular and
> udp_with_checksum_irregular.
> 
> cheers,
> /anders
> 
> 
> 
> _______________________________________________
> Rohc mailing list
> [email protected]
> https://www1.ietf.org/mailman/listinfo/rohc