[ovs-dev] [PATCH 4/4] datapath-windows: Conntrack - Enable FTP support

Sairam Venugopal vsairam at vmware.com
Tue Dec 6 19:07:30 UTC 2016


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" <aserdean at 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_openvswitc
>h_ovs_blob_master_lib_netlink.c-23L649&d=DgIFAg&c=uilaK90D4TOVoH58JNXRgQ&r
>=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=7UoJ195CXB4akaVdcnobT2i4Y2D
>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;
>> +        }
>> +    }
>> 



More information about the dev mailing list