[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