rohc team mailing list archive

[Didier Barvaux <didier@xxxxxxxxxxx>]

FWX,

>    To complete the RoHC library, I add code to support profile 0x0006 
> (RoHC-TCP) in the current trunk (rev 335).
> 
>   I send it to request 
> comments/tests/corrections/validations/suggestions ...


Hey, I (finally) get time this evening to fully read the TCP patch!

It's a _huge_ work, FWX! Thank you!

It needs some corrections (see my comments/questions/notes below), but
it's a great first version.

I must say that I didn't test it for the moment, as there is some work
to get it up to date with the current trunk (my fault). I'll do the
merge ASAP. I'll do it in a dedicated branch, then when it will be
ready we will merge it in the trunk.

Below are my comments/questions/notes. They might a little rough (they
are notes after all :). Do not take them personally. Please
comment/answer if you feel the need.

Regards,
Didier


==== general ====

 - I removed ESP from the patch because it was already merged


==== common ==== 

=> src/common/ipproto.h:

 - why a (partial) list of IP protocol numbers?

=> tcp_profile/common/tcp.h:

 - why are the *_(TO|FROM)_MPTR macro for?
 - why defining the base_header_ip_t, multi_ptr_t, WB_t and LWB_t
   unions?

=> src/common/trace.[ch]:

 - I think I will enhance src/common/rohc_traces.[ch] with that files,
   are you OK?


==== compression ====

=> src/comp/c_rohc_lib.[ch]

 - The filename seems too general, tcp_encoding.[ch] seems better.
   The methods defined in RFC 4997 could even be split in a new file  
   named rfc4997_encoding.[ch].
 - c_lsb(): according to RFC 4997 section 4.11.5, it seems that the
   'lsb' encoding scheme matches the W-LSB one already defined in
   src/common/wlsb.c. 
 - c_ip_id_lsb(): could be merged with code from c_generic.c
 - lsb_masks: I prefer avoiding global variables...
 - unit tests should be written for these encoding functions

=> src/comp/c_tcp.c

 - private functions should be made static
 - c_tcp_create(): the code that parse IP headers in IP/UDP/RTP
   profiles could probably be re-used (and maybe enhanced if needed)
 - the call to random() must be replaced by a call to the dedicated 
   callback function in the library
 - c_tcp_check_context(): the code from c_generic could be re-used
 - c_tcp_encode(): the code that parse IP headers in IP/UDP/RTP
   profiles could probably be re-used (and maybe enhanced if needed)
 - rohc_v2_code_dynamic_ipv6_option_part(): why "v2" ?
 - tcp_changed_tcp_dynamic(): unused at the moment
 - c_ts_lsb(): could probably be moved in c_rohc_lib.[ch]
 - c_sack_pure_lsb(): same as c_ts_lsb()
 - c_sack_block(): same as c_ts_lsb()
 - c_tcp_opt_sack(): same as c_ts_lsb()
 - tcp_compress_tcp_options(): goto is OK only for error handling, the
   function should be reworked to get rid of  
   'new_index_with_compressed_value' and 'same_index_without_value'.
 - co_baseheader(): same as tcp_compress_tcp_options()

=> src/comp/c_tcp.h

 - almost all the structs and functions should not be public


==== decompression ====

=> src/decomp/d_rohc_lib.[ch]

 - lsb_xor_masks: global variable should be avoided if possible
 - d_lsb(): seems very basic in comparison to the the lsb_decode module.
 - variable_length_32_size: global variable should be avoided if
   possible
 - The filename seems too general, tcp_decoding.[ch] seems better.
   The methods defined in RFC 4997 could even be split in a new file  
   named rfc4997_encoding.[ch].
 - d_ip_id_lsb(): could be merged with ip_id_offset_decode module

=> src/decomp/d_tcp.c

 - private functions should be made static
 - my_ip_fast_csum(): duplicate ip_fast_csum() from src/common/ip.h
 - d_tcp_decode_ir(): should be merged with d_tcp_decode() (because of a
   change between 1.4.0 and 1.5.0 releases, my fault...)
 - tcp_get_static_size(), tcp_detect_ir_size(),
   tcp_detect_ir_dyn_size(), and tcp_detect_tcp_dynamic_size(): should
   be removed (because of a change between 1.4.0 and 1.5.0 releases, my
   fault...)
 - d_size_ts_lsb(): looks like duplicated code with sdvl_decode().
 - d_sack_pure_lsb(): move in d_rohc_lib.[ch] ?
 - d_sack_block(): move in d_rohc_lib.[ch] ?
 - d_tcp_opt_sack(): move in d_rohc_lib.[ch] ?

=> src/decomp/d_tcp.h

 - almost all the structs and functions should not be public
 - PACKET_TCP_* should be added to enum rohc_packet_t in rohc_packets.h
 - d_tcp_decode_CO(): the rnd_X/seq_X detection could be put in a
   tcp_detect_packet_type() function.
 - d_tcp_decode_CO(): goto is OK only for error handling, the function
   should be reworked

==== end ====

Attachment: signature.asc
Description: PGP signature