| Message ID | 20170130163713.17524-4-aschultz@tpip.net |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 30, 2017 at 05:37:10PM +0100, Andreas Schultz wrote: > This unifies duplicate code into a helper. It also prepares the > groundwork to add a lookup version that uses the socket to find > attache pdp contexts. > > Signed-off-by: Andreas Schultz <aschultz@tpip.net> > --- > drivers/net/gtp.c | 120 +++++++++++++++++++++++------------------------------- > 1 file changed, 51 insertions(+), 69 deletions(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index c96c71f..6b7a3c2 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c [...] > +static struct pdp_ctx *gtp_genl_find_pdp(struct sk_buff *skb, > + struct genl_info *info) > +{ > + struct pdp_ctx *pctx; > + > + if (info->attrs[GTPA_LINK]) > + pctx = gtp_genl_find_pdp_by_link(skb, info); > + else > + pctx = ERR_PTR(-EINVAL); > + if (!pctx) > + pctx = ERR_PTR(-ENOENT); > + > + return pctx; > +} For gtp_genl_find_pdp(), I think this is easier to read: if (!info->attrs[GTPA_LINK]) return ERR_PTR(-EINVAL); pctx = gtp_genl_find_pdp_by_link(skb, info); if (!pctx) return ERR_PTR(-ENOENT); return pctx;
----- On Feb 2, 2017, at 3:19 PM, pablo pablo@netfilter.org wrote: > On Mon, Jan 30, 2017 at 05:37:10PM +0100, Andreas Schultz wrote: >> This unifies duplicate code into a helper. It also prepares the >> groundwork to add a lookup version that uses the socket to find >> attache pdp contexts. >> >> Signed-off-by: Andreas Schultz <aschultz@tpip.net> >> --- >> drivers/net/gtp.c | 120 +++++++++++++++++++++++------------------------------- >> 1 file changed, 51 insertions(+), 69 deletions(-) >> >> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c >> index c96c71f..6b7a3c2 100644 >> --- a/drivers/net/gtp.c >> +++ b/drivers/net/gtp.c > [...] >> +static struct pdp_ctx *gtp_genl_find_pdp(struct sk_buff *skb, >> + struct genl_info *info) >> +{ >> + struct pdp_ctx *pctx; >> + >> + if (info->attrs[GTPA_LINK]) >> + pctx = gtp_genl_find_pdp_by_link(skb, info); >> + else >> + pctx = ERR_PTR(-EINVAL); >> + if (!pctx) >> + pctx = ERR_PTR(-ENOENT); >> + >> + return pctx; >> +} > > For gtp_genl_find_pdp(), I think this is easier to read: > > if (!info->attrs[GTPA_LINK]) > return ERR_PTR(-EINVAL); > > pctx = gtp_genl_find_pdp_by_link(skb, info); > if (!pctx) > return ERR_PTR(-ENOENT); > > return pctx; Yes, but a later patch (will be submitted after this series is accepted) will change that to: if (info->attrs[GTPA_LINK]) pctx = gtp_genl_find_pdp_by_link(skb, info); else if (info->attrs[GTPA_FD]) pctx = gtp_genl_find_pdp_by_socket(skb, info); else pctx = ERR_PTR(-EINVAL); if (!pctx) pctx = ERR_PTR(-ENOENT); return pctx; I can use your form for this change, but have a larger change later. Which way do you prefer it? Andreas
On Thu, Feb 02, 2017 at 03:27:17PM +0100, Andreas Schultz wrote: > > > ----- On Feb 2, 2017, at 3:19 PM, pablo pablo@netfilter.org wrote: > > > On Mon, Jan 30, 2017 at 05:37:10PM +0100, Andreas Schultz wrote: > >> This unifies duplicate code into a helper. It also prepares the > >> groundwork to add a lookup version that uses the socket to find > >> attache pdp contexts. > >> > >> Signed-off-by: Andreas Schultz <aschultz@tpip.net> > >> --- > >> drivers/net/gtp.c | 120 +++++++++++++++++++++++------------------------------- > >> 1 file changed, 51 insertions(+), 69 deletions(-) > >> > >> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > >> index c96c71f..6b7a3c2 100644 > >> --- a/drivers/net/gtp.c > >> +++ b/drivers/net/gtp.c > > [...] > >> +static struct pdp_ctx *gtp_genl_find_pdp(struct sk_buff *skb, > >> + struct genl_info *info) > >> +{ > >> + struct pdp_ctx *pctx; > >> + > >> + if (info->attrs[GTPA_LINK]) > >> + pctx = gtp_genl_find_pdp_by_link(skb, info); > >> + else > >> + pctx = ERR_PTR(-EINVAL); > >> + if (!pctx) > >> + pctx = ERR_PTR(-ENOENT); > >> + > >> + return pctx; > >> +} > > > > For gtp_genl_find_pdp(), I think this is easier to read: > > > > if (!info->attrs[GTPA_LINK]) > > return ERR_PTR(-EINVAL); > > > > pctx = gtp_genl_find_pdp_by_link(skb, info); > > if (!pctx) > > return ERR_PTR(-ENOENT); > > > > return pctx; > > Yes, but a later patch (will be submitted after this series is > accepted) will change that to: > > if (info->attrs[GTPA_LINK]) > pctx = gtp_genl_find_pdp_by_link(skb, info); > else if (info->attrs[GTPA_FD]) > pctx = gtp_genl_find_pdp_by_socket(skb, info); > else > pctx = ERR_PTR(-EINVAL); > > if (!pctx) > pctx = ERR_PTR(-ENOENT); > > return pctx; > > I can use your form for this change, but have a larger change > later. Which way do you prefer it? I see, then leave this as it is.
On Mon, Jan 30, 2017 at 05:37:10PM +0100, Andreas Schultz wrote: > Signed-off-by: Andreas Schultz <aschultz@tpip.net> Acked-by: Harald Welte <laforge@gnumonks.org>
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index c96c71f..6b7a3c2 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -1027,47 +1027,62 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info) return err; } +static struct pdp_ctx *gtp_genl_find_pdp_by_link(struct sk_buff *skb, + struct genl_info *info) +{ + struct gtp_dev *gtp; + + gtp = gtp_genl_find_dev(sock_net(skb->sk), info->attrs); + if (!gtp) + return ERR_PTR(-ENODEV); + + if (info->attrs[GTPA_MS_ADDRESS]) { + __be32 ip = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]); + + return ipv4_pdp_find(gtp, ip); + } else if (info->attrs[GTPA_VERSION]) { + u32 gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]); + + if (gtp_version == GTP_V0 && info->attrs[GTPA_TID]) + return gtp0_pdp_find(gtp, nla_get_u64( + info->attrs[GTPA_TID])); + else if (gtp_version == GTP_V1 && info->attrs[GTPA_I_TEI]) + return gtp1_pdp_find(gtp, nla_get_u32( + info->attrs[GTPA_I_TEI])); + } + + return ERR_PTR(-EINVAL); +} + +static struct pdp_ctx *gtp_genl_find_pdp(struct sk_buff *skb, + struct genl_info *info) +{ + struct pdp_ctx *pctx; + + if (info->attrs[GTPA_LINK]) + pctx = gtp_genl_find_pdp_by_link(skb, info); + else + pctx = ERR_PTR(-EINVAL); + + if (!pctx) + pctx = ERR_PTR(-ENOENT); + + return pctx; +} + static int gtp_genl_del_pdp(struct sk_buff *skb, struct genl_info *info) { struct pdp_ctx *pctx; - struct gtp_dev *gtp; int err = 0; - if (!info->attrs[GTPA_VERSION] || - !info->attrs[GTPA_LINK]) + if (!info->attrs[GTPA_VERSION]) return -EINVAL; rcu_read_lock(); - gtp = gtp_genl_find_dev(sock_net(skb->sk), info->attrs); - if (!gtp) { - err = -ENODEV; - goto out_unlock; - } - - switch (nla_get_u32(info->attrs[GTPA_VERSION])) { - case GTP_V0: - if (!info->attrs[GTPA_TID]) { - err = -EINVAL; - goto out_unlock; - } - pctx = gtp0_pdp_find(gtp, nla_get_u64(info->attrs[GTPA_TID])); - break; - case GTP_V1: - if (!info->attrs[GTPA_I_TEI]) { - err = -EINVAL; - goto out_unlock; - } - pctx = gtp1_pdp_find(gtp, nla_get_u64(info->attrs[GTPA_I_TEI])); - break; - - default: - err = -EINVAL; - goto out_unlock; - } - - if (!pctx) { - err = -ENOENT; + pctx = gtp_genl_find_pdp(skb, info); + if (IS_ERR(pctx)) { + err = PTR_ERR(pctx); goto out_unlock; } @@ -1129,49 +1144,16 @@ static int gtp_genl_get_pdp(struct sk_buff *skb, struct genl_info *info) { struct pdp_ctx *pctx = NULL; struct sk_buff *skb2; - struct gtp_dev *gtp; - u32 gtp_version; int err; - if (!info->attrs[GTPA_VERSION] || - !info->attrs[GTPA_LINK]) + if (!info->attrs[GTPA_VERSION]) return -EINVAL; - gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]); - switch (gtp_version) { - case GTP_V0: - case GTP_V1: - break; - default: - return -EINVAL; - } - rcu_read_lock(); - gtp = gtp_genl_find_dev(sock_net(skb->sk), info->attrs); - if (!gtp) { - err = -ENODEV; - goto err_unlock; - } - - if (gtp_version == GTP_V0 && - info->attrs[GTPA_TID]) { - u64 tid = nla_get_u64(info->attrs[GTPA_TID]); - - pctx = gtp0_pdp_find(gtp, tid); - } else if (gtp_version == GTP_V1 && - info->attrs[GTPA_I_TEI]) { - u32 tid = nla_get_u32(info->attrs[GTPA_I_TEI]); - - pctx = gtp1_pdp_find(gtp, tid); - } else if (info->attrs[GTPA_MS_ADDRESS]) { - __be32 ip = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]); - - pctx = ipv4_pdp_find(gtp, ip); - } - - if (pctx == NULL) { - err = -ENOENT; + pctx = gtp_genl_find_pdp(skb, info); + if (IS_ERR(pctx)) { + err = PTR_ERR(pctx); goto err_unlock; }
This unifies duplicate code into a helper. It also prepares the groundwork to add a lookup version that uses the socket to find attache pdp contexts. Signed-off-by: Andreas Schultz <aschultz@tpip.net> --- drivers/net/gtp.c | 120 +++++++++++++++++++++++------------------------------- 1 file changed, 51 insertions(+), 69 deletions(-)