I don't strongly object the current version either (with the fix
above), but this version seems simpler.
Is it worth while to make it inline?
Acked-by: Andy Zhou <az...@nicira.com>
On Thu, Dec 18, 2014 at 5:11 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> Thinking about this a little more, I could make the debug interface
> set a single static 'enable_ufid' variable, then have a helper
> function like so:
>
> +static bool
> +udpif_use_ufid(struct udpif *udpif)
> +{
> + return enable_ufid && ofproto_dpif_get_enable_ufid(udpif->backer);
> +}
>
> I don't feel strongly about it, though this is perhaps a little more
> intuitive.
>
> On 18 December 2014 at 16:42, Joe Stringer <joestrin...@nicira.com> wrote:
>> Almost; theoretically not all udpifs will have the same backer, each
>> of which may or may not support UFID. So, it's still useful to track
>> this per-udpif. (One example is right now if you add a userspace
>> datapath and a kernel datapath, the userspace will support UFID but
>> the kernel will not.)
>>
>> The rest I think can be implemented by just performing the check
>> before setting udpif->enable_ufid to true in the debug interface,
>> incremental below:
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index f0cd4cc..46e6ce0 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -2087,11 +2087,20 @@ upcall_unixctl_enable_ufid(struct unixctl_conn
>> *conn, int argc OVS_UNUSED,
>> const char *argv[] OVS_UNUSED, void *aux
>> OVS_UNUSED)
>> {
>> struct udpif *udpif;
>> + bool error = false;
>>
>> LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
>> - atomic_store(&udpif->enable_ufid, true);
>> + if (ofproto_dpif_get_enable_ufid(udpif->backer)) {
>> + atomic_store(&udpif->enable_ufid, true);
>> + } else {
>> + error = true;
>> + }
>> + }
>> + if (error) {
>> + unixctl_command_reply_error(conn, "One or more datapaths
>> don't support UFID");
>> + } else {
>> + unixctl_command_reply(conn, "Datapath dumping tersely using
>> UFID enabled");
>> }
>> - unixctl_command_reply(conn, "Datapath dumping tersely using UFID
>> enabled");
>> }
>>
>> /* Set the flow limit.
>>
>> On 18 December 2014 at 16:15, Andy Zhou <az...@nicira.com> wrote:
>>> It seems we can do away with udpif->enable_ufid field. udpif has
>>> access to the backer, so it can know
>>> about the datapath capability.
>>>
>>> The debug interface just need to control another global 'enable'
>>> variable. ufid can only be use if both conditions
>>> are true. This will also remove the possibility of turning on udpif
>>> when the datapath does not support it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
