Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Fri, Sep 01, 2017 at 12:58:59PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 01, 2017 at 12:50:49PM +0200, Phil Sutter wrote:
> > On Fri, Sep 01, 2017 at 12:17:33PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> > > > Add these two new functions to set up netlink sockets in the global
> > > > context structure.
> > > 
> > > We can alternatively call this nft_ctx_netlink_auto() if prefer.
> > > 
> > > I'm just trying to skip the type/flag field for nft_ctx_alloc().
> > > 
> > > Does this look acceptable to you to have this extra API to request
> > > libnftables to deal with IO details too?
> > 
> > I think we could do it in a simpler way:
> > 
> > | /* create an mnl netlink socket object */
> > | struct mnl_socket *netlink_open_sock(void);
> > | 
> > | /* create nft context, optionally passing mnl socket object returned
> > |  * from netlink_open_sock()
> > |  * Calling nft_ctx_new(NULL) is equivalent to calling
> > |  * nft_ctx_new(netlink_open_sock())
> > |  */
> > | static struct nft_ctx *nft_ctx_new(struct mnl_socket *nf_sock);
> > 
> > This way we allow the application to control mnl_socket object, provide
> > a simple API for applications which don't need that and at the same time
> > always have ctx->nf_sock point to the socket so we can further simplify
> > things.
> > 
> > What do you think?
> 
> I don't want to expose the mnl_socket for the simple API.

Well, technically my proposal changes nft_ctx_new() to be a shared
function between simple and advanced API. Simple users passing NULL as
parameter won't have to take care of socket init and therefore don't
have a separate mnl_socket object they could manipulate.

> Once we expose the socket, we're allowing people to modify behaviour
> via setsockopt() toggles. Then, we'll have to tell people not to
> modify socket options for the simple API, or it can be worst: someone
> will try to fix this by adding lots of branches to deal with every
> socket option combination in libnftables.
> 
> So this is an intentional design decision here.
> 
> For people willing to do their own netlink IO, we should push them to
> use the advanced API.

Your patches seem to make the simple API unnecessarily complicated by
forcing users to call nft_ctx_netlink_init(). Why not keep this in
nft_ctx_new() and provide an alternative to struct nft_ctx for advanced
API? This way simple and advanced functions could be easily identified
by the type of their first parameter.

Maybe I'm also overlooking something regarding advanced API - can you
maybe give an example of a set of function declarations it would consist
of?

Thanks, Phil