[ovs-dev] [PATCH 0/3] Initial support for new SIP Alg.

Darrell Ball dball at vmware.com
Fri Jan 5 06:40:50 UTC 2018


Thanks for the series/work; I’ll be reviewing this series, but focusing on the high level aspects initially.

Some high level comments:

I noticed that your own comments in the series often pointed out various issues with this series, such as 
assuming TCP transport, which is ‘unusual’, no NAT support, single media session only, lack of testing support etc.

Although SIP does not store as much dynamic state as other algs, it still needs to have a separate hidden portion of memory
for that. Embedding that in struct conn at top level is ‘not ideal’.

As I mentioned I would in the below referred thread, I sent a patch to support more algs, including SIP.
https://patchwork.ozlabs.org/patch/853010/
My patch is generic plumbing for additional algs, including NAT aspects, for internal future alg requirements and SIP.
It will help this series and remove a bunch of duplicated code.

Patch 2: sip_delete_conn() should not exist; when a conn is deleted, just check for SIP state to clean and call a SIP function
              Same idea for sip_set_conn_state()
              handle_sip() should be in conntrack-sip.c
              sip_expectation_create() – remove function and use my patch, calling with correct  arguments since it is more
                                                          general and handles NAT.

Patch 3 should not be a separate patch; it is basic SIP and clearer to fold into patch 2.

I did not review Patch 1 yet in enough depth to provide useful comments.

BTW, I already have some testing infra for SIP, but I am not sure I will submit soon since I am doing something else at the
moment. I’ll hold off on this for now.

Maybe an RFC label would be better initially.

Darrell


On 12/22/17, 11:54 AM, "ovs-dev-bounces at openvswitch.org on behalf of Tiago Lam" <ovs-dev-bounces at openvswitch.org on behalf of tiagolam at gmail.com> wrote:

    This patch-set is an initial approach at implementing the new SIP Alg,
    mentioned by Aaron at [1].
    
    I'm mostly interested in getting to know your thoughts of how this is
    headed. There are a couple of points that are worth bringing up:
    - As mentioned in patches 1/3 and 2/3, this is still a preliminary
      implementation, and some work will be needed to move away from some
      assuptions, like assuming the SIP traffic is always going over IPv4
      and TCP;
    - At the moment, the sip state is being stored in the conn struct. I
      followed the example of seq_skew_dir here, which is also stored there,
      but realise this is not ideal. It seems storing it somewhere agnostic
      will be ideal in the future, to avoid polluting that struct with
      different Alg's details;

Embed it deeper into struct conn as pointer to SIP stuff


    - The SIP helpers functions and structures are in conntrack-sip.h and
      conntrack-sip.c. This can create confusion when comparing to
      conntrack-tcp.c and other protocols since SIP is an Alg and is at a
      different level.

SIP is complex and deserves separate files.
I don’t think it is confusing – it is fine.

    
    With regards to testing, for now, this has been tested manually, by
    setting up the flows mentioned in patch 2/3 and having two VMs connected
    to OvS, both using SIPp to simulate real traffic both ways. I'm going to
    have a look at how this can be automated and added to
    tests/system-traffic.at, together with the rest of the already existing
    tests.
    
    [1] [CONNTRACK] Discussions at OvS 2017:
        https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DNovember_341089.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=DDHX2MTCsXS7GD8ie27aEdUDgGRK2EIntHQAxtrkWmI&s=md5csJDVqD97O6SvpYWNjbuQZYN2sfYKe4cF1-dzt1A&e=
    
    Tiago Lam (3):
      Conntrack: Add new API for future SIP Alg.
      Conntrack: Add initial support for new SIP Alg.
      Conntrack: Support asymmetric RTP port for SIP.
    
     include/openvswitch/ofp-actions.h |   4 +
     lib/automake.mk                   |   2 +
     lib/conntrack-private.h           |   2 +
     lib/conntrack-sip.c               | 491 ++++++++++++++++++++++++++++++++++++++
     lib/conntrack-sip.h               | 123 ++++++++++
     lib/conntrack.c                   | 254 +++++++++++++++++++-
     lib/ofp-parse.c                   |   5 +
     ofproto/ofproto-dpif-xlate.c      |   3 +
     8 files changed, 883 insertions(+), 1 deletion(-)
     create mode 100644 lib/conntrack-sip.c
     create mode 100644 lib/conntrack-sip.h
    
    -- 
    2.14.3
    
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=DDHX2MTCsXS7GD8ie27aEdUDgGRK2EIntHQAxtrkWmI&s=ouDOBTdmowKrm1jxoMzaRvMz2-dkf_rTN2DyWa6_P_c&e=
    







More information about the dev mailing list