← Back to team overview

rohc team mailing list archive

Re: Support of RFC4996 RoHC-TCP

 

 Hello Didier,

 Thank you for all your question about the support of TCP in RoHC.

 I answer bellow, preceding by -->> symbols.

 Best regards,
                FWX.

Le 21/10/2012 19:33, Didier Barvaux a écrit :
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!

-->> Very nice ! Thanks.

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

-->> months of work, yes !


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

-->> Yes, it's why I ask you help and comment.


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.


-->> Ok. No Problem.

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.


-->> Ok Didier.


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?

-->> Because others IP protocol numbers are defined in other include files, with compiler for example. Here are only those not found elsewhere.

=> tcp_profile/common/tcp.h:

  - why are the *_(TO|FROM)_MPTR macro for?

-->> For ARM support, because of alignment problem.

  - why defining the base_header_ip_t, multi_ptr_t, WB_t and LWB_t
    unions?

-->> With this method, you have a single pointer to a lot of different types of datas. In the IP header, you don't know, at the begin, what you'll find : TCP, RTP, etc... WB_t and LWB_t are types defined for use when you need to access datas as bytes table or as word (dword).


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

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


-->> Ok. At the begining, it was just an internal tool for debug. But it may help others.


==== 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].

-->> Ok for rfc4997_encoding.[ch]. And I'll create also rfc5225_encoding.[ch] for RoHCv2 encoding methods.

  - 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.

-->> Perhaps, but I prefer use my code, for the moment.

  - c_ip_id_lsb(): could be merged with code from c_generic.c

-->> Same answer. My code is more short and quick than the other...

  - lsb_masks: I prefer avoiding global variables...

-->> Ok, but how to use same variables in compression and decompression source files, without duplicate them ? Put them in a C file in common ?

  - unit tests should be written for these encoding functions

-->> I'm agree with you. But all existing tests are passed ok.


=> src/comp/c_tcp.c

  - private functions should be made static

-->> Ok.

  - c_tcp_create(): the code that parse IP headers in IP/UDP/RTP
    profiles could probably be re-used (and maybe enhanced if needed)

-->> Yes, it will be a good idea, in rohc_compress.c for example, or c_generic.c. I'll try to write a good code, that can be enhanced.

  - the call to random() must be replaced by a call to the dedicated
    callback function in the library

-->> Which callback function?

  - c_tcp_check_context(): the code from c_generic could be re-used

-->> Can you be more precise?

  - c_tcp_encode(): the code that parse IP headers in IP/UDP/RTP
    profiles could probably be re-used (and maybe enhanced if needed)

-->> Yes.

  - rohc_v2_code_dynamic_ipv6_option_part(): why "v2" ?

-->> For historical reason... I code these functions for RoHC V2 support, then copy them in the TCP support. Code is the same.

  - tcp_changed_tcp_dynamic(): unused at the moment

-->> Yes, because I need your help to code this function. (see DBX comment inside).

  - c_ts_lsb(): could probably be moved in c_rohc_lib.[ch]

-->> It is more easy to have this function near the code where they are called. And they are specific to TCP compression. May be moved when code will be exact. But it is not urgent.

  - c_sack_pure_lsb(): same as c_ts_lsb()

-->> Same answer.

  - c_sack_block(): same as c_ts_lsb()

-->> Same answer.

  - c_tcp_opt_sack(): same as c_ts_lsb()

-->> Same answer.

  - 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'.

-->> I have not enough experience to change this code. But why not in the future.

  - co_baseheader(): same as tcp_compress_tcp_options()


-->> Ok.

=> src/comp/c_tcp.h

  - almost all the structs and functions should not be public


-->> Ok.


==== decompression ====

=> src/decomp/d_rohc_lib.[ch]

  - lsb_xor_masks: global variable should be avoided if possible

-->> Ok.

  - d_lsb(): seems very basic in comparison to the the lsb_decode module.

-->> Yes, but give me an example where there is a problem ...

  - variable_length_32_size: global variable should be avoided if
    possible

-->> Ok.

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

-->> I prefer rfc4997_decoding.[ch], according with comp/rfc4997_encoding.[ch]

  - d_ip_id_lsb(): could be merged with ip_id_offset_decode module

-->> Later, if possible.


=> src/decomp/d_tcp.c

  - private functions should be made static

-->> Ok again.

  - my_ip_fast_csum(): duplicate ip_fast_csum() from src/common/ip.h

-->> Yes, but without alignment problem with ARM. Shorter and more quick. The other code doesn't work with all platforms. I'll suggest a patch to use my code, rather then the other.

  - 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...)

-->> No idea. I'll look this point.

  - 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...)

-->> Same answer.

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

-->> Same response than for c_ts_lsb(), c_sack_pure_lsb(), c_sack_block() or c_tcp_opt_sack(): later, when code will be validated.


=> src/decomp/d_tcp.h

  - almost all the structs and functions should not be public

-->> Ok.

  - PACKET_TCP_* should be added to enum rohc_packet_t in rohc_packets.h

-->> There is different types of TCP packets. Not usefull, because only used in d_tcp.c file.

  - d_tcp_decode_CO(): the rnd_X/seq_X detection could be put in a
    tcp_detect_packet_type() function.

-->> Perhaps.

  - d_tcp_decode_CO(): goto is OK only for error handling, the function
    should be reworked

-->> I'm agree with you. I'm waiting for tests to do that.


==== end ====


-->> Ouf. Thanks for all Didier.
I'll add this code, with correction, in the current branch, as soon as possible.


_______________________________________________
Mailing list: https://launchpad.net/~rohc
Post to     : rohc@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~rohc
More help   : https://help.launchpad.net/ListHelp



Follow ups

References