Re: [nft RFC PATCH 6/6] src: add events reporting

[Patrick McHardy <kaber@trash.net>]

On Tue, Feb 18, 2014 at 10:28:11AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 18, 2014 at 02:03:55AM +0000, Patrick McHardy wrote:
> > On Tue, Feb 18, 2014 at 02:10:07AM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Feb 18, 2014 at 12:18:38AM +0100, Arturo Borrero Gonzalez wrote:
> > > > +static int netlink_events_table_cb(const struct nlmsghdr *nlh, int type,
> > > > +				   struct netlink_ev_handler *evh)
> > > > +{
> > > > +	struct nft_table *nlt;
> > > > +	uint32_t family;
> > > > +	char buf[4096];
> > > > +
> > > > +	nlt = nft_table_alloc();
> > > > +	if (nlt == NULL)
> > > > +		memory_allocation_error();
> > > > +
> > > > +	if (nft_table_nlmsg_parse(nlh, nlt) < 0) {
> > > > +		netlink_io_error(evh->ctx, evh->loc,
> > > > +				"Could not parse table: %s",
> > > > +				strerror(errno));
> > > 
> > > I think you should exit on parsing errors.
> > 
> > I'm not so sure for event reporting. We should abort reporting the current
> > event, sure. But I don't see why we shouldn't continue listering for further
> > events.
> 
> I think if we fail to parse anything it means that there's some bug
> that need to be fixed, eg. someone changed the length of an attribute
> so validation fails. I think stopping there should help us to get
> people reporting problems.

Well, it depends on what exactly we couldn't parse or handle. Basic things
like table names etc. can of course trigger a fatal error. But unknown
expression types shouldn't cause an error. Seemingly invalid ways
epressions are linked etc should probably also not do that since they
may simply be created by something that is not nft and we don't understand
them. So it really depends on where the error is happening.