rohc team mailing list archive

[Didier Barvaux <didier@xxxxxxxxxxx>]

FWX,

>   To complete the RoHC library, I add code to support profile 0x0003 
> (ESP: Encapsulating Security Payload) in the current trunk.
> 
>   The special case when the NULL encryption algorithm is used, is not 
> supported in this version.

I got enough time to read your patch (thanks again for it!). Please
find hereafter some questions/notes.

Please do not make any change to you patch for the moment. I already
rebased your patch to the current trunk tree, and it would be a shame
to do the work twice :)


1/ SN not required at the end of base header?

  The 16-bit SN field at the end of the base header seems to be only
  required for IP-only, UDP and UDP-Lite profiles, not the RTP and ESP
  ones. 

  Section 5.12.1 of RFC 3095 states:
    In contrast to ROHC UDP, no extra sequence number is added to the
    dynamic part of the ESP header: the ESP sequence number is the only
    element.

  So, on compression side, part 8 of the code_IR_packet() function
  should test for the ESP profile in addition to the RTP profile. So
  does part 7 of the code_IR_DYN_packet() function.

  On decompression side, esp_decode_dynamic_esp() should call the
  ip_decode_dynamic_ip() because it decodes an extra 16-bit SN field.


2/ LSB shift value?

  The LSB shift "p" value for the ESP profile seems to be the same as
  for the RTP profile, not the one of the IP-only, non-RTP UDP and
  non-RTP UDP-Lite profiles.

  Section 5.12 of RFC 3095 states:
    The interpretation interval (value of p) for the ESP-based SN is as
    with ROHC RTP (profile 0x0001).

  So, in c_generic_create(), the switch seems wrong. On decompression
  side this is a problem too. Also, the esp_detect_ir_size() seems to
  return 2 bytes more than expected.


3/ ESP-specific context required?

  You defined an ESP-specific context that stores the SN of the ESP
  header. The ESP SN is the main SN (MSN). The MSN is stored in the
  generic context. I think you should use the g_context->sn. It would
  avoid duplication, and remove the need for an ESP-specific context.
  What do you think of?


Regards,
Didier Barvaux

Attachment: signature.asc
Description: PGP signature