| Message ID | 1372315398-19683-3-git-send-email-amwang@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 27-06-2013 10:43, Cong Wang wrote: > Signed-off-by: Cong Wang <amwang@redhat.com> > --- > include/net/inet_addr.h | 20 ++++++++++++++++++++ > net/core/netpoll.c | 24 ++---------------------- > 2 files changed, 22 insertions(+), 22 deletions(-) > diff --git a/include/net/inet_addr.h b/include/net/inet_addr.h > index 66a16fe..1379287 100644 > --- a/include/net/inet_addr.h > +++ b/include/net/inet_addr.h > @@ -4,6 +4,7 @@ > #include <linux/in.h> > #include <linux/in6.h> > #include <linux/socket.h> > +#include <linux/inet.h> > #include <net/addrconf.h> > > union inet_addr { > @@ -59,4 +60,23 @@ static inline bool inet_addr_multicast(const union inet_addr *ipa) > } > #endif > > +static inline int inet_pton(const char *str, union inet_addr *addr) > +{ > + const char *end; > + > + if (!strchr(str, ':') && > + in4_pton(str, -1, (void *)addr, -1, &end) > 0) { > + if (!*end) > + return 0; > + } > + if (in6_pton(str, -1, addr->sin6.sin6_addr.s6_addr, -1, &end) > 0) { > +#if IS_ENABLED(CONFIG_IPV6) > + if (!*end) How about: if (IS_ENABLED(CONFIG_IPV6) && !*end) > + return 1; > +#else > + return -1; > +#endif > + } > + return -1; > +} > #endif WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-06-27 at 14:43 +0800, Cong Wang wrote: > Signed-off-by: Cong Wang <amwang@redhat.com> > --- > include/net/inet_addr.h | 20 ++++++++++++++++++++ > net/core/netpoll.c | 24 ++---------------------- > 2 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/include/net/inet_addr.h b/include/net/inet_addr.h > index 66a16fe..1379287 100644 > --- a/include/net/inet_addr.h > +++ b/include/net/inet_addr.h > @@ -4,6 +4,7 @@ > #include <linux/in.h> > #include <linux/in6.h> > #include <linux/socket.h> > +#include <linux/inet.h> > #include <net/addrconf.h> > > union inet_addr { > @@ -59,4 +60,23 @@ static inline bool inet_addr_multicast(const union inet_addr *ipa) > } > #endif > > +static inline int inet_pton(const char *str, union inet_addr *addr) > +{ > + const char *end; > + > + if (!strchr(str, ':') && > + in4_pton(str, -1, (void *)addr, -1, &end) > 0) { > + if (!*end) > + return 0; > + } > + if (in6_pton(str, -1, addr->sin6.sin6_addr.s6_addr, -1, &end) > 0) { > +#if IS_ENABLED(CONFIG_IPV6) > + if (!*end) > + return 1; > +#else > + return -1; > +#endif > + } > + return -1; > +} [...] The return values for this are awful. I think the return value should be 0 for success or -EINVAL on error. The caller can then check addr->sa.sa_family if they want to know the address family immediately. Ben.
On Thu, 27 Jun 2013 14:43:15 +0800 Cong Wang <amwang@redhat.com> wrote: > Signed-off-by: Cong Wang <amwang@redhat.com> > --- > include/net/inet_addr.h | 20 ++++++++++++++++++++ > net/core/netpoll.c | 24 ++---------------------- > 2 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/include/net/inet_addr.h b/include/net/inet_addr.h > index 66a16fe..1379287 100644 > --- a/include/net/inet_addr.h > +++ b/include/net/inet_addr.h > @@ -4,6 +4,7 @@ > #include <linux/in.h> > #include <linux/in6.h> > #include <linux/socket.h> > +#include <linux/inet.h> > #include <net/addrconf.h> > > union inet_addr { > @@ -59,4 +60,23 @@ static inline bool inet_addr_multicast(const union inet_addr *ipa) > } > #endif > > +static inline int inet_pton(const char *str, union inet_addr *addr) > +{ > A couple of comments: 1. No reason for this to be inline 2. If function has same name as userspace it must have same arguments and return value. Either: a. rename it to kinet_pton or some other name b. make it work the same. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/27/2013 06:18 PM, Sergei Shtylyov wrote: >> Signed-off-by: Cong Wang <amwang@redhat.com> >> --- >> include/net/inet_addr.h | 20 ++++++++++++++++++++ >> net/core/netpoll.c | 24 ++---------------------- >> 2 files changed, 22 insertions(+), 22 deletions(-) >> diff --git a/include/net/inet_addr.h b/include/net/inet_addr.h >> index 66a16fe..1379287 100644 >> --- a/include/net/inet_addr.h >> +++ b/include/net/inet_addr.h [...] >> @@ -59,4 +60,23 @@ static inline bool inet_addr_multicast(const union >> inet_addr *ipa) >> } >> #endif >> >> +static inline int inet_pton(const char *str, union inet_addr *addr) >> +{ >> + const char *end; >> + >> + if (!strchr(str, ':') && >> + in4_pton(str, -1, (void *)addr, -1, &end) > 0) { >> + if (!*end) >> + return 0; >> + } >> + if (in6_pton(str, -1, addr->sin6.sin6_addr.s6_addr, -1, &end) > 0) { >> +#if IS_ENABLED(CONFIG_IPV6) >> + if (!*end) > How about: > if (IS_ENABLED(CONFIG_IPV6) && !*end) >> + return 1; >> +#else Sorry, managed to miss #else... #if could be avoided anyway, tho this could require an extra patch. >> + return -1; >> +#endif >> + } >> + return -1; >> +} >> #endif WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-06-27 at 16:32 +0100, Ben Hutchings wrote: > The return values for this are awful. I think the return value should > be 0 for success or -EINVAL on error. > > The caller can then check addr->sa.sa_family if they want to know the > address family immediately. Indeed, I will fix it. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-06-27 at 14:51 -0700, Stephen Hemminger wrote: > > > > +static inline int inet_pton(const char *str, union inet_addr *addr) > > +{ > > > > A couple of comments: > 1. No reason for this to be inline Okay, I will move them into net/core/utils.c. > 2. If function has same name as userspace it must have same arguments > and return value. Either: > a. rename it to kinet_pton or some other name > b. make it work the same. > Makes sense too me, I will try your option b) first, if it is over-kill I will fall back to option a). Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/inet_addr.h b/include/net/inet_addr.h index 66a16fe..1379287 100644 --- a/include/net/inet_addr.h +++ b/include/net/inet_addr.h @@ -4,6 +4,7 @@ #include <linux/in.h> #include <linux/in6.h> #include <linux/socket.h> +#include <linux/inet.h> #include <net/addrconf.h> union inet_addr { @@ -59,4 +60,23 @@ static inline bool inet_addr_multicast(const union inet_addr *ipa) } #endif +static inline int inet_pton(const char *str, union inet_addr *addr) +{ + const char *end; + + if (!strchr(str, ':') && + in4_pton(str, -1, (void *)addr, -1, &end) > 0) { + if (!*end) + return 0; + } + if (in6_pton(str, -1, addr->sin6.sin6_addr.s6_addr, -1, &end) > 0) { +#if IS_ENABLED(CONFIG_IPV6) + if (!*end) + return 1; +#else + return -1; +#endif + } + return -1; +} #endif diff --git a/net/core/netpoll.c b/net/core/netpoll.c index c300358..6631b5e 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -907,26 +907,6 @@ void netpoll_print_options(struct netpoll *np) } EXPORT_SYMBOL(netpoll_print_options); -static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr) -{ - const char *end; - - if (!strchr(str, ':') && - in4_pton(str, -1, (void *)addr, -1, &end) > 0) { - if (!*end) - return 0; - } - if (in6_pton(str, -1, addr->sin6.sin6_addr.s6_addr, -1, &end) > 0) { -#if IS_ENABLED(CONFIG_IPV6) - if (!*end) - return 1; -#else - return -1; -#endif - } - return -1; -} - int netpoll_parse_options(struct netpoll *np, char *opt) { char *cur=opt, *delim; @@ -946,7 +926,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt) if ((delim = strchr(cur, '/')) == NULL) goto parse_failed; *delim = 0; - ipv6 = netpoll_parse_ip_addr(cur, &np->local_ip); + ipv6 = inet_pton(cur, &np->local_ip); if (ipv6 < 0) goto parse_failed; else @@ -982,7 +962,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt) if ((delim = strchr(cur, '/')) == NULL) goto parse_failed; *delim = 0; - ipv6 = netpoll_parse_ip_addr(cur, &np->remote_ip); + ipv6 = inet_pton(cur, &np->remote_ip); if (ipv6 < 0) goto parse_failed; else if (np->ipv6 != (bool)ipv6)
Signed-off-by: Cong Wang <amwang@redhat.com> --- include/net/inet_addr.h | 20 ++++++++++++++++++++ net/core/netpoll.c | 24 ++---------------------- 2 files changed, 22 insertions(+), 22 deletions(-)