Re: [PATCH nf-next v2] nft_osf: Add ttl option support

[Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx>]

On 10/3/18 5:36 PM, Pablo Neira Ayuso wrote:
> Hi Fernando,
>
> A few comments.
>
> On Sat, Sep 29, 2018 at 12:18:51PM +0200, Fernando Fernandez Mancera wrote:
> > Add ttl option support to the nftables "osf" expression.
[..]
> >   	if (!os_name)
> >   		strncpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN);
> >   	else
> > @@ -46,6 +50,15 @@ static int nft_osf_init(const struct nft_ctx *ctx,
> >   {
> >   	struct nft_osf *priv = nft_expr_priv(expr);
> >   	int err;
> > +	u8 ttl;
> > +
> > +	if (!tb[NFTA_OSF_TTL])
> > +		return -EINVAL;
> > +
> > +	ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
> > +	if (ttl > 2)
> > +		return -EOPNOTSUPP;
> > +	priv->ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
>
> Better make this optional, ie.
>
>          if (tb[NFTA_OSF_TTL]) {
>                  ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
>                  if (ttl > 2)
>                          return -EINVAL;
>
>                  priv->ttl = ttl;
>          }

Seems fine to me, I am going to do it.

> What is the good default value for priv->ttl? We were using -1 before
> this patch, but now ttl is u8, while nf_osf_ttl() takes a signed
> value. Better make this s8 instead of u8?
>
> Thanks.

About the default value for priv->ttl, I think we can use 0 instead of 
-1. Right now, if priv->ttl is -1 the behaviour established is "-m osf 
--ttl 0". What do you think about that?

Thanks.