Unified code for the threading part sounds good.
Can you please add an item to https://github.com/openvswitch/ovs-issues/issues
so we can tackle it later on?
I'm okay with leaving the init/cleanup as it is for the moment.
Thanks,
Alin.
> -Original Message-
> From: Sairam Venugopal [mailto:vsai...@vmware.com]
> Sent: Tuesday, December 6, 2016 9:08 PM
> To: Alin Serdean <aserd...@cloudbasesolutions.com>;
> d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 4/4] datapath-windows: Conntrack - Enable
> FTP support
>
> I feel that these additional threads that are spawned for the lifetime of the
> driver ought to be visible in the Driver load/unload functions. This will be
> beneficial for both debugging and lifecycle management. I agree that
> encapsulating it under Conntrack.c will make the code contained, but might
> abstract it out when triaging failures.
>
> Given the number of new threads (and the subsequent init/cleanups) that
> have been introduced recently, I was thinking of writing a wrapper to
> consolidate most of the boilerplate code. It will be a module that manages
> thread Initialize/Cleanup/Add entry/Delete entry. Stt, Conntrack, Conntrack-
> related (in-review), IP-Fragmentation (WIP) will be some of the modules that
> can be cleaned up. In this case, we can simply spawn all of these threads in
> OvsCreateSwitch() and destroy them in OvsExtDetach(). I will send out a
> design document for this and we can take it up for discussion.
>
> Thanks,
> Sairam
>
> On 12/5/16, 10:49 AM, "Alin Serdean" <aserd...@cloudbasesolutions.com>
> wrote:
>
> >Thanks for the patch.
> >
> >I think OvsInitCtRelated/ OvsCleanupCtRelated would make more sense to
> >be inside OvsInitConntrack/OvsCleanupConntrack since the functionality
> >are tied together.
> >One small nit.
> >
> >Thanks,
> >Alin.
> >> +ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_HELPER);
> >> +if (ctAttr) {
> >> +helper = NlAttrGet(ctAttr);
> >> +if (!memchr(helper, '\0', 16)) {
> >[Alin Serdean] We must be careful here, because the size may differ(i.e.
> >a message could be forged). I think we should add
> >https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_openvsw
> >itc
> >h_ovs_blob_master_lib_netlink.c-
> 23L649=DgIFAg=uilaK90D4TOVoH58JNXRg
> >Q
> >=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo=7UoJ195CXB4a
> kaVdcnobT2i4
> >Y2D fX-PunuSALxVk-eg=gmM1JR66iLF1xYDzdLTr22ReSlvwRWcBhJ-
> E7XpKceo=
> >to the windows datapath and use it.
> >> +OVS_LOG_ERROR("Invalid CT_ATTR_HELPER:%s", helper);
> >> +return NDIS_STATUS_INVALID_PARAMETER;
> >> +}
> >> +if (strcmp("ftp", helper) != 0) {
> >> +/* Only support FTP */
> >> +return NDIS_STATUS_NOT_SUPPORTED;
> >> +}
> >> +}
> >>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
