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&d=DgIFAg&c=uilaK90D4TOVoH58JNXRg > >Q&r > >=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=7UoJ195CXB4a > kaVdcnobT2i4 > >Y2D fX-PunuSALxVk-eg&s=gmM1JR66iLF1xYDzdLTr22ReSlvwRWcBhJ- > E7XpKceo&e= > >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
