Hi Darrell,
Thanks for your initial review.
I'll work on incorporating your comments into v2 (which I'm planning to
submit already with some tests). A couple of small comments are in-line.
On 01/05/2018 06:40 AM, Darrell Ball wrote:
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.
That was going to be part of my next iteration as well - I've seen you
just sent v3 of the series, so I'll work based on that.
I mainly wanted to get what I had "out there" in order to start syncing,
so worked mainly based on master (and the previous "conntrack: Alg
improvements." patch that was merged back in December,
https://patchwork.ozlabs.org/cover/844311/).
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.
Sure, I'll merge that (3/3) into 2/3 in v2 as well.
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.
Good point. Thought of doing it but then missed it when actually
sending. Will do in v2, thanks.
Darrell
On 12/22/17, 11:54 AM, "ovs-dev-boun...@openvswitch.org on behalf of Tiago Lam"
<ovs-dev-boun...@openvswitch.org on behalf of tiago...@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
That's what I've at the moment, having set a sip_state struct for that.
Tiago.
- 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=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
