[ovs-dev] [PATCH] ipfix: allow empty targets column in table IPFIX

[Ben Pfaff]

On Mon, Nov 11, 2013 at 01:29:56PM -0800, Romain Lenglet wrote:
> ----- Original Message -----
> > From: "Ben Pfaff" <blp at nicira.com>
> > To: "Romain Lenglet" <rlenglet at vmware.com>
> > Cc: dev at openvswitch.org
> > Sent: Monday, November 11, 2013 12:42:55 PM
> > Subject: Re: [ovs-dev] [PATCH] ipfix: allow empty targets column in table IPFIX
> > 
> > On Tue, Nov 05, 2013 at 11:24:59AM -0800, Romain Lenglet wrote:
> > > Signed-off-by: Romain Lenglet <rlenglet at vmware.com>
> > 
> > What happens if the new schema is used with the old bridge.c, and some
> > row actually has an empty targets column?  If it's something bad then
> > we should backport the bridge.c change (without the schema change)
> > because it's certainly possible that someone mismatches schemas by
> > accident.
> 
> That column can currently never be empty.
> ovsdb implicitly adds an empty string "" into that column if no
> value is given, because it has a minimum size of 1.
> No connection can be opened to a target with address "", so the whole IPFIX row is disabled (an IPFIX exporter is enabled only if we can open connections to all the target collectors in its row's targets).
> Until that empty string is explicitly removed from the column by the user, the row remains disabled.
> 
> That's correct ovsdb behavior, but it proved to be unintuitive to users.
> 
> Removing the min:1 constraint will not impact the behavior for any existing row, whether it has a "" target or not.
> 
> This patch just avoids unnecessary trouble for users who insert IPFIX rows with no targets:
> - there won't be log messages anymore due to failed connections to target "";
> - they won't need to manually remove the "" target after row insertion.
> 
> 
> If one uses the new schema with an old bridge.c and creates new IPFIX rows with no targets, it's highly probably it will segfault (didn't try though).
> What branches should I backport the bridge.c changes to?

Thanks for the analysis.  I understand the situation now.

I forgot that in fact ovs-vswitchd will check the row contents against
the schema compiled into it, which means that even if ovsdb-server
allows an empty column, ovs-vswitchd will reject it, protecting the
ofproto-dpif-ipfix module against the data that it is not prepared to
handle.  So there is in fact no new risk to the ipfix module.

Would you mind adding some of the above content to the commit message,
then resubmitting this patch along with the other one?  (I think that
they depend on each other anyhow.)

Thanks,

Ben.