| Message ID | 20201107153139.3552-4-andrea.mayer@uniroma2.it (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/5] vrf: add mac header for tunneled packets when sniffer is attached | expand |
On Sat, 7 Nov 2020 16:31:37 +0100 Andrea Mayer wrote: > We introduce two callbacks used for customizing the creation/destruction of > a SRv6 behavior. Such callbacks are defined in the new struct > seg6_local_lwtunnel_ops and hereafter we provide a brief description of > them: > > - build_state(...): used for calling the custom constructor of the > behavior during its initialization phase and after all the attributes > have been parsed successfully; > > - destroy_state(...): used for calling the custom destructor of the > behavior before it is completely destroyed. > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> Looks good, minor nits. > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > index 63a82e2fdea9..4b0f155d641d 100644 > --- a/net/ipv6/seg6_local.c > +++ b/net/ipv6/seg6_local.c > @@ -33,11 +33,23 @@ > > struct seg6_local_lwt; > > +typedef int (*slwt_build_state_t)(struct seg6_local_lwt *slwt, const void *cfg, > + struct netlink_ext_ack *extack); > +typedef void (*slwt_destroy_state_t)(struct seg6_local_lwt *slwt); Let's avoid the typedefs. Instead of taking a pointer to the op take a pointer to the ops struct in seg6_local_lwtunnel_build_state() etc. > +/* callbacks used for customizing the creation and destruction of a behavior */ > +struct seg6_local_lwtunnel_ops { > + slwt_build_state_t build_state; > + slwt_destroy_state_t destroy_state; > +}; > + > struct seg6_action_desc { > int action; > unsigned long attrs; > int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt); > int static_headroom; > + > + struct seg6_local_lwtunnel_ops slwt_ops; > }; > > struct bpf_lwt_prog { > @@ -1015,6 +1027,45 @@ static void destroy_attrs(struct seg6_local_lwt *slwt) > __destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt); > } > > +/* call the custom constructor of the behavior during its initialization phase > + * and after that all its attributes have been parsed successfully. > + */ > +static int > +seg6_local_lwtunnel_build_state(struct seg6_local_lwt *slwt, const void *cfg, > + struct netlink_ext_ack *extack) > +{ > + slwt_build_state_t build_func; > + struct seg6_action_desc *desc; > + int err = 0; > + > + desc = slwt->desc; > + if (!desc) > + return -EINVAL; This is impossible, right? > + > + build_func = desc->slwt_ops.build_state; > + if (build_func) > + err = build_func(slwt, cfg, extack); > + > + return err; no need for err, just use return directly. if (!ops->build_state) return 0; return ops->build_state(...); > +} > + > +/* call the custom destructor of the behavior which is invoked before the > + * tunnel is going to be destroyed. > + */ > +static void seg6_local_lwtunnel_destroy_state(struct seg6_local_lwt *slwt) > +{ > + slwt_destroy_state_t destroy_func; > + struct seg6_action_desc *desc; > + > + desc = slwt->desc; > + if (!desc) > + return; > + > + destroy_func = desc->slwt_ops.destroy_state; > + if (destroy_func) > + destroy_func(slwt); > +} > + > static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) > { > struct seg6_action_param *param; > @@ -1090,8 +1141,16 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, > > err = parse_nla_action(tb, slwt); > if (err < 0) > + /* In case of error, the parse_nla_action() takes care of > + * releasing resources which have been acquired during the > + * processing of attributes. > + */ that's the normal behavior for a kernel function, comment is unnecessary IMO > goto out_free; > > + err = seg6_local_lwtunnel_build_state(slwt, cfg, extack); > + if (err < 0) > + goto free_attrs; The function is called destroy_attrs, call the label out_destroy_attrs, or err_destroy_attrs. > newts->type = LWTUNNEL_ENCAP_SEG6_LOCAL; > newts->flags = LWTUNNEL_STATE_INPUT_REDIRECT; > newts->headroom = slwt->headroom; > @@ -1100,6 +1159,9 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, > > return 0; > > +free_attrs: > + destroy_attrs(slwt); > + no need for empty lines on error paths > out_free: > kfree(newts); > return err; > @@ -1109,6 +1171,8 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt) > { > struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt); > > + seg6_local_lwtunnel_destroy_state(slwt); > + > destroy_attrs(slwt); > > return;
Hi Jakub, many thanks for your review. Please see my responses inline: On Tue, 10 Nov 2020 14:56:55 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 7 Nov 2020 16:31:37 +0100 Andrea Mayer wrote: > > We introduce two callbacks used for customizing the creation/destruction of > > a SRv6 behavior. Such callbacks are defined in the new struct > > seg6_local_lwtunnel_ops and hereafter we provide a brief description of > > them: > > > > - build_state(...): used for calling the custom constructor of the > > behavior during its initialization phase and after all the attributes > > have been parsed successfully; > > > > - destroy_state(...): used for calling the custom destructor of the > > behavior before it is completely destroyed. > > > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> > > Looks good, minor nits. > > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > > index 63a82e2fdea9..4b0f155d641d 100644 > > --- a/net/ipv6/seg6_local.c > > +++ b/net/ipv6/seg6_local.c > > @@ -33,11 +33,23 @@ > > > > struct seg6_local_lwt; > > > > +typedef int (*slwt_build_state_t)(struct seg6_local_lwt *slwt, const void *cfg, > > + struct netlink_ext_ack *extack); > > +typedef void (*slwt_destroy_state_t)(struct seg6_local_lwt *slwt); > > Let's avoid the typedefs. Instead of taking a pointer to the op take a > pointer to the ops struct in seg6_local_lwtunnel_build_state() etc. > Ok, I will do it this way in v3. > > +/* callbacks used for customizing the creation and destruction of a behavior */ > > +struct seg6_local_lwtunnel_ops { > > + slwt_build_state_t build_state; > > + slwt_destroy_state_t destroy_state; > > +}; > > + > > struct seg6_action_desc { > > int action; > > unsigned long attrs; > > int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt); > > int static_headroom; > > + > > + struct seg6_local_lwtunnel_ops slwt_ops; > > }; > > > > struct bpf_lwt_prog { > > @@ -1015,6 +1027,45 @@ static void destroy_attrs(struct seg6_local_lwt *slwt) > > __destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt); > > } > > > > +/* call the custom constructor of the behavior during its initialization phase > > + * and after that all its attributes have been parsed successfully. > > + */ > > +static int > > +seg6_local_lwtunnel_build_state(struct seg6_local_lwt *slwt, const void *cfg, > > + struct netlink_ext_ack *extack) > > +{ > > + slwt_build_state_t build_func; > > + struct seg6_action_desc *desc; > > + int err = 0; > > + > > + desc = slwt->desc; > > + if (!desc) > > + return -EINVAL; > > This is impossible, right? > Yes, it is. I will remove this check in v3. > > + > > + build_func = desc->slwt_ops.build_state; > > + if (build_func) > > + err = build_func(slwt, cfg, extack); > > + > > + return err; > > no need for err, just use return directly. > > if (!ops->build_state) > return 0; > return ops->build_state(...); > Ok, I will do it in this way in v3. > > +} > > + > > +/* call the custom destructor of the behavior which is invoked before the > > + * tunnel is going to be destroyed. > > + */ > > +static void seg6_local_lwtunnel_destroy_state(struct seg6_local_lwt *slwt) > > +{ > > + slwt_destroy_state_t destroy_func; > > + struct seg6_action_desc *desc; > > + > > + desc = slwt->desc; > > + if (!desc) > > + return; > > + > > + destroy_func = desc->slwt_ops.destroy_state; > > + if (destroy_func) > > + destroy_func(slwt); > > +} > > + > > static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) > > { > > struct seg6_action_param *param; > > @@ -1090,8 +1141,16 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, > > > > err = parse_nla_action(tb, slwt); > > if (err < 0) > > + /* In case of error, the parse_nla_action() takes care of > > + * releasing resources which have been acquired during the > > + * processing of attributes. > > + */ > > that's the normal behavior for a kernel function, comment is > unnecessary IMO > Yes and this is the way it should be. But before this patch, the parse_nla_action() in case of error did not always release all the acquired resources. From this patcheset onward, the parse_nla_action() behaves like we expect. Therefore, I will remove the comment in v3. > > goto out_free; > > > > + err = seg6_local_lwtunnel_build_state(slwt, cfg, extack); > > + if (err < 0) > > + goto free_attrs; > > The function is called destroy_attrs, call the label out_destroy_attrs, > or err_destroy_attrs. > Fine, I will stick with the out_destroy_attrs to be consistent and uniform with the out_free label in v3. > > newts->type = LWTUNNEL_ENCAP_SEG6_LOCAL; > > newts->flags = LWTUNNEL_STATE_INPUT_REDIRECT; > > newts->headroom = slwt->headroom; > > @@ -1100,6 +1159,9 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, > > > > return 0; > > > > +free_attrs: > > + destroy_attrs(slwt); > > + > > no need for empty lines on error paths > Ok. > > out_free: > > kfree(newts); > > return err; > > @@ -1109,6 +1171,8 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt) > > { > > struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt); > > > > + seg6_local_lwtunnel_destroy_state(slwt); > > + > > destroy_attrs(slwt); > > > > return; > Thank you, Andrea
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index 63a82e2fdea9..4b0f155d641d 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -33,11 +33,23 @@ struct seg6_local_lwt; +typedef int (*slwt_build_state_t)(struct seg6_local_lwt *slwt, const void *cfg, + struct netlink_ext_ack *extack); +typedef void (*slwt_destroy_state_t)(struct seg6_local_lwt *slwt); + +/* callbacks used for customizing the creation and destruction of a behavior */ +struct seg6_local_lwtunnel_ops { + slwt_build_state_t build_state; + slwt_destroy_state_t destroy_state; +}; + struct seg6_action_desc { int action; unsigned long attrs; int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt); int static_headroom; + + struct seg6_local_lwtunnel_ops slwt_ops; }; struct bpf_lwt_prog { @@ -1015,6 +1027,45 @@ static void destroy_attrs(struct seg6_local_lwt *slwt) __destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt); } +/* call the custom constructor of the behavior during its initialization phase + * and after that all its attributes have been parsed successfully. + */ +static int +seg6_local_lwtunnel_build_state(struct seg6_local_lwt *slwt, const void *cfg, + struct netlink_ext_ack *extack) +{ + slwt_build_state_t build_func; + struct seg6_action_desc *desc; + int err = 0; + + desc = slwt->desc; + if (!desc) + return -EINVAL; + + build_func = desc->slwt_ops.build_state; + if (build_func) + err = build_func(slwt, cfg, extack); + + return err; +} + +/* call the custom destructor of the behavior which is invoked before the + * tunnel is going to be destroyed. + */ +static void seg6_local_lwtunnel_destroy_state(struct seg6_local_lwt *slwt) +{ + slwt_destroy_state_t destroy_func; + struct seg6_action_desc *desc; + + desc = slwt->desc; + if (!desc) + return; + + destroy_func = desc->slwt_ops.destroy_state; + if (destroy_func) + destroy_func(slwt); +} + static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) { struct seg6_action_param *param; @@ -1090,8 +1141,16 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, err = parse_nla_action(tb, slwt); if (err < 0) + /* In case of error, the parse_nla_action() takes care of + * releasing resources which have been acquired during the + * processing of attributes. + */ goto out_free; + err = seg6_local_lwtunnel_build_state(slwt, cfg, extack); + if (err < 0) + goto free_attrs; + newts->type = LWTUNNEL_ENCAP_SEG6_LOCAL; newts->flags = LWTUNNEL_STATE_INPUT_REDIRECT; newts->headroom = slwt->headroom; @@ -1100,6 +1159,9 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla, return 0; +free_attrs: + destroy_attrs(slwt); + out_free: kfree(newts); return err; @@ -1109,6 +1171,8 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt) { struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt); + seg6_local_lwtunnel_destroy_state(slwt); + destroy_attrs(slwt); return;
We introduce two callbacks used for customizing the creation/destruction of a SRv6 behavior. Such callbacks are defined in the new struct seg6_local_lwtunnel_ops and hereafter we provide a brief description of them: - build_state(...): used for calling the custom constructor of the behavior during its initialization phase and after all the attributes have been parsed successfully; - destroy_state(...): used for calling the custom destructor of the behavior before it is completely destroyed. Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> --- net/ipv6/seg6_local.c | 64 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)