[PATCH 0/2] constify nf_hook_ops structures

[PATCH 0/2] constify nf_hook_ops structures

[Julia Lawall]

The nf_hook_ops structure is only passed as the second argument to
nf_register_net_hook or nf_unregister_net_hook, both of which are
declared as const.  Thus the nf_hook_ops structure itself can be
const.

Done with the help of Coccinelle.

---

 net/decnet/netfilter/dn_rtmsg.c    |    2 +-
 net/ipv4/netfilter/ipt_CLUSTERIP.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

[PATCH 1/2] decnet: dn_rtmsg: constify nf_hook_ops structures

[Julia Lawall]

The nf_hook_ops structure is only passed as the second argument to
nf_register_net_hook or nf_unregister_net_hook, both of which are
declared as const.  Thus the nf_hook_ops structure itself can be
const.

Done with the help of Coccinelle.

// <smpl>
@r disable optional_qualifier@
identifier i;
position p;
@@
static struct nf_hook_ops i@p = { ... };

@ok1@
identifier r.i;
expression e;
position p;
@@
 \(nf_register_net_hook\|nf_unregister_net_hook\)(e,&i@p)

@bad@
position p != {r.p,ok1.p};
identifier r.i;
struct nf_hook_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
 struct nf_hook_ops i = { ... };
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 net/decnet/netfilter/dn_rtmsg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c
index aa8ffec..ab395e5 100644
--- a/net/decnet/netfilter/dn_rtmsg.c
+++ b/net/decnet/netfilter/dn_rtmsg.c
@@ -115,7 +115,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb)
 	RCV_SKB_FAIL(-EINVAL);
 }
 
-static struct nf_hook_ops dnrmg_ops __read_mostly = {
+static const struct nf_hook_ops dnrmg_ops = {
 	.hook		= dnrmg_hook,
 	.pf		= NFPROTO_DECNET,
 	.hooknum	= NF_DN_ROUTE,

[PATCH 2/2] netfilter: ipt_CLUSTERIP: constify nf_hook_ops structures

[Julia Lawall]

The nf_hook_ops structure is only passed as the second argument to
nf_register_net_hook or nf_unregister_net_hook, both of which are
declared as const.  Thus the nf_hook_ops structure itself can be
const.

Done with the help of Coccinelle.

// <smpl>
@r disable optional_qualifier@
identifier i;
position p;
@@
static struct nf_hook_ops i@p = { ... };

@ok1@
identifier r.i;
expression e;
position p;
@@
 \(nf_register_net_hook\|nf_unregister_net_hook\)(e,&i@p)

@bad@
position p != {r.p,ok1.p};
identifier r.i;
struct nf_hook_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
 struct nf_hook_ops i = { ... };
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index efaa04d..17b4ca5 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -625,7 +625,7 @@ static void arp_print(struct arp_payload *payload)
 	return NF_ACCEPT;
 }
 
-static struct nf_hook_ops cip_arp_ops __read_mostly = {
+static const struct nf_hook_ops cip_arp_ops = {
 	.hook = arp_mangle,
 	.pf = NFPROTO_ARP,
 	.hooknum = NF_ARP_OUT,

Re: [PATCH 0/2] constify nf_hook_ops structures

[Florian Westphal]

Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> The nf_hook_ops structure is only passed as the second argument to
> nf_register_net_hook or nf_unregister_net_hook, both of which are
> declared as const.  Thus the nf_hook_ops structure itself can be
> const.

Right, also see
http://patchwork.ozlabs.org/patch/793767/

This series misses most of them (all arrays perhaps)?

Re: [PATCH 0/2] constify nf_hook_ops structures

[Julia Lawall]

On Sat, 29 Jul 2017, Florian Westphal wrote:

> Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > The nf_hook_ops structure is only passed as the second argument to
> > nf_register_net_hook or nf_unregister_net_hook, both of which are
> > declared as const.  Thus the nf_hook_ops structure itself can be
> > const.
>
> Right, also see
> http://patchwork.ozlabs.org/patch/793767/
>
> This series misses most of them (all arrays perhaps)?

Yes, my rule doesn't look for arrays.  I guess they are all done already
anyway?

thanks,
julia


> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/2] constify nf_hook_ops structures

[Florian Westphal]

Julia Lawall <julia.lawall@lip6.fr> wrote:
> 
> 
> On Sat, 29 Jul 2017, Florian Westphal wrote:
> 
> > Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > > The nf_hook_ops structure is only passed as the second argument to
> > > nf_register_net_hook or nf_unregister_net_hook, both of which are
> > > declared as const.  Thus the nf_hook_ops structure itself can be
> > > const.
> >
> > Right, also see
> > http://patchwork.ozlabs.org/patch/793767/
> >
> > This series misses most of them (all arrays perhaps)?
> 
> Yes, my rule doesn't look for arrays.  I guess they are all done already
> anyway?

I think so (the patch is not yet applied though).

From a quick glance I don't see why we can't e.g. constify
nf_conntrack_l3/4_proto too. It is not going to be as simple
as just placing const everywhere, but I see no requirement for
having these writeable.

Re: [PATCH 0/2] constify nf_hook_ops structures

[Julia Lawall]

On Sat, 29 Jul 2017, Florian Westphal wrote:

> Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Sat, 29 Jul 2017, Florian Westphal wrote:
> >
> > > Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > > > The nf_hook_ops structure is only passed as the second argument to
> > > > nf_register_net_hook or nf_unregister_net_hook, both of which are
> > > > declared as const.  Thus the nf_hook_ops structure itself can be
> > > > const.
> > >
> > > Right, also see
> > > http://patchwork.ozlabs.org/patch/793767/
> > >
> > > This series misses most of them (all arrays perhaps)?
> >
> > Yes, my rule doesn't look for arrays.  I guess they are all done already
> > anyway?
>
> I think so (the patch is not yet applied though).

OK, just drop my patch then.

>
> From a quick glance I don't see why we can't e.g. constify
> nf_conntrack_l3/4_proto too. It is not going to be as simple
> as just placing const everywhere, but I see no requirement for
> having these writeable.

I will take a look.

thanks,
julia

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/2] constify nf_hook_ops structures

[Florian Westphal]

Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sat, 29 Jul 2017, Florian Westphal wrote:
> > From a quick glance I don't see why we can't e.g. constify
> > nf_conntrack_l3/4_proto too. It is not going to be as simple
> > as just placing const everywhere, but I see no requirement for
> > having these writeable.
> 
> I will take a look.

Thanks.

nf_logger and nf_loginfo also look like constify candidates.

If there is a way to add "const" qualifier to pointer-to-structs
that are not modified this would good as well to have IMO, if just
for purpose of documentation.  For instance:

+++ b/net/netfilter/nf_conntrack_core.c
@@ -1177,8 +1177,8 @@ void nf_conntrack_free(struct nf_conn *ct)
 static noinline struct nf_conntrack_tuple_hash *
 init_conntrack(struct net *net, struct nf_conn *tmpl,
               const struct nf_conntrack_tuple *tuple,
-              struct nf_conntrack_l3proto *l3proto,
-              struct nf_conntrack_l4proto *l4proto,
+              const struct nf_conntrack_l3proto *l3proto,
+              const struct nf_conntrack_l4proto *l4proto,


(its only passed as arg to a function that expects
"const struct nf_conntrack_x *").

I think we have several (also non-static helpers) that
take "struct foo *" arg while they could use "const struct foo*"
instead.

Re: [PATCH 0/2] constify nf_hook_ops structures

[Julia Lawall]

On Sat, 29 Jul 2017, Florian Westphal wrote:

> Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sat, 29 Jul 2017, Florian Westphal wrote:
> > > From a quick glance I don't see why we can't e.g. constify
> > > nf_conntrack_l3/4_proto too. It is not going to be as simple
> > > as just placing const everywhere, but I see no requirement for
> > > having these writeable.
> >
> > I will take a look.
>
> Thanks.

For the protos, the functions nf_ct_l3proto_register and
nf_ct_l4proto_register_one update the nla_size field. I don't know how
many structures reach these functions.

julia

>
> nf_logger and nf_loginfo also look like constify candidates.
>
> If there is a way to add "const" qualifier to pointer-to-structs
> that are not modified this would good as well to have IMO, if just
> for purpose of documentation.  For instance:
>
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1177,8 +1177,8 @@ void nf_conntrack_free(struct nf_conn *ct)
>  static noinline struct nf_conntrack_tuple_hash *
>  init_conntrack(struct net *net, struct nf_conn *tmpl,
>                const struct nf_conntrack_tuple *tuple,
> -              struct nf_conntrack_l3proto *l3proto,
> -              struct nf_conntrack_l4proto *l4proto,
> +              const struct nf_conntrack_l3proto *l3proto,
> +              const struct nf_conntrack_l4proto *l4proto,
>
>
> (its only passed as arg to a function that expects
> "const struct nf_conntrack_x *").
>
> I think we have several (also non-static helpers) that
> take "struct foo *" arg while they could use "const struct foo*"
> instead.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>