Re: final follow-up review of last-call

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