| Message ID | 1415957459-11458-4-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2014-11-14 12:30, Maxim Uvarov wrote: > Define API and implement promisc functions: > odp_pktio_set_promisc() > odp_pktio_promisc() > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/api/odp_packet_io.h | 25 +++++++ > platform/linux-generic/odp_packet_io.c | 77 ++++++++++++++++++++++ > 2 files changed, 102 insertions(+) > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h > index e5cf320..87589ce 100644 > --- a/platform/linux-generic/include/api/odp_packet_io.h > +++ b/platform/linux-generic/include/api/odp_packet_io.h > @@ -159,6 +159,31 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > int odp_pktio_mtu(odp_pktio_t id); > > /** > + * Set promiscuous mode on a packet IO interface. > + * > + * @param[in] id ODP packet IO handle. > + * @param[in] enable 1 to enable promiscuous mode, 0 to disable > + * > + * @retval 0 on success. > + * @retval -1 on a bad pktio id > + * @retval -1 ilegal enable value > + * @retval -1 any other error > + */ > +int odp_pktio_set_promisc(odp_pktio_t id, int enable); I think it is clearer with two functions. void odp_pktio_set_promisc(odp_pktio_t id) and odp_pktio_clear_promisc(odp_pktio_t id) we should do this also for other functions of this kind. The main problem of the present approach is that we don't know if val is a boolean without looking at the documentation. Cheers, Anders > + > +/** > + * Determine if promiscuous mode is enabled for a packet IO interface. > + * > + * @param[in] id ODP packet IO handle. > + * > + * @retval 1 if promiscuous mode is enabled. > + * @retval 0 if promiscuous mode is disabled. > + * @retval -1 on a bad pktio id > + * @retval -1 any other error > +*/ > +int odp_pktio_promisc(odp_pktio_t id); odp_pktio_is_promisc Cheers, Anders
I disagree. The setters for booleans are simpler, cleaner, and more programmer-friendly as a single API. We don't need to create more APIs. It makes for more test cases and more complicated code since with little benefit. On Wed, Nov 19, 2014 at 3:54 AM, Anders Roxell <anders.roxell@linaro.org> wrote: > On 2014-11-14 12:30, Maxim Uvarov wrote: > > Define API and implement promisc functions: > > odp_pktio_set_promisc() > > odp_pktio_promisc() > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > --- > > platform/linux-generic/include/api/odp_packet_io.h | 25 +++++++ > > platform/linux-generic/odp_packet_io.c | 77 > ++++++++++++++++++++++ > > 2 files changed, 102 insertions(+) > > > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h > b/platform/linux-generic/include/api/odp_packet_io.h > > index e5cf320..87589ce 100644 > > --- a/platform/linux-generic/include/api/odp_packet_io.h > > +++ b/platform/linux-generic/include/api/odp_packet_io.h > > @@ -159,6 +159,31 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > > int odp_pktio_mtu(odp_pktio_t id); > > > > /** > > + * Set promiscuous mode on a packet IO interface. > > + * > > + * @param[in] id ODP packet IO handle. > > + * @param[in] enable 1 to enable promiscuous mode, 0 to disable > > + * > > + * @retval 0 on success. > > + * @retval -1 on a bad pktio id > > + * @retval -1 ilegal enable value > > + * @retval -1 any other error > > + */ > > +int odp_pktio_set_promisc(odp_pktio_t id, int enable); > > I think it is clearer with two functions. > > void odp_pktio_set_promisc(odp_pktio_t id) and > odp_pktio_clear_promisc(odp_pktio_t id) > > we should do this also for other functions of this kind. > > The main problem of the present approach is that we don't know if val is > a boolean without looking at the documentation. > > Cheers, > Anders > > > + > > +/** > > + * Determine if promiscuous mode is enabled for a packet IO interface. > > + * > > + * @param[in] id ODP packet IO handle. > > + * > > + * @retval 1 if promiscuous mode is enabled. > > + * @retval 0 if promiscuous mode is disabled. > > + * @retval -1 on a bad pktio id > > + * @retval -1 any other error > > +*/ > > +int odp_pktio_promisc(odp_pktio_t id); > > odp_pktio_is_promisc > > Cheers, > Anders > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On Wed, Nov 19, 2014 at 2:18 PM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > I disagree. The setters for booleans are simpler, cleaner, and more programmer-friendly as a single API. We don't need to create more APIs. It makes for more test cases and more complicated code since with little benefit. I second Bill on this. As soon as you look at the API and see the second argument you should start wondering what it's needed for. > > > On Wed, Nov 19, 2014 at 3:54 AM, Anders Roxell <anders.roxell@linaro.org> wrote: >> >> On 2014-11-14 12:30, Maxim Uvarov wrote: >> > Define API and implement promisc functions: >> > odp_pktio_set_promisc() >> > odp_pktio_promisc() >> > >> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> > --- >> > platform/linux-generic/include/api/odp_packet_io.h | 25 +++++++ >> > platform/linux-generic/odp_packet_io.c | 77 ++++++++++++++++++++++ >> > 2 files changed, 102 insertions(+) >> > >> > diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h >> > index e5cf320..87589ce 100644 >> > --- a/platform/linux-generic/include/api/odp_packet_io.h >> > +++ b/platform/linux-generic/include/api/odp_packet_io.h >> > @@ -159,6 +159,31 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); >> > int odp_pktio_mtu(odp_pktio_t id); >> > >> > /** >> > + * Set promiscuous mode on a packet IO interface. >> > + * >> > + * @param[in] id ODP packet IO handle. >> > + * @param[in] enable 1 to enable promiscuous mode, 0 to disable >> > + * >> > + * @retval 0 on success. >> > + * @retval -1 on a bad pktio id >> > + * @retval -1 ilegal enable value >> > + * @retval -1 any other error >> > + */ >> > +int odp_pktio_set_promisc(odp_pktio_t id, int enable); >> >> I think it is clearer with two functions. >> >> void odp_pktio_set_promisc(odp_pktio_t id) and >> odp_pktio_clear_promisc(odp_pktio_t id) >> >> we should do this also for other functions of this kind. >> >> The main problem of the present approach is that we don't know if val is >> a boolean without looking at the documentation. >> >> Cheers, >> Anders >> >> > + >> > +/** >> > + * Determine if promiscuous mode is enabled for a packet IO interface. >> > + * >> > + * @param[in] id ODP packet IO handle. >> > + * >> > + * @retval 1 if promiscuous mode is enabled. >> > + * @retval 0 if promiscuous mode is disabled. >> > + * @retval -1 on a bad pktio id >> > + * @retval -1 any other error >> > +*/ >> > +int odp_pktio_promisc(odp_pktio_t id); >> >> odp_pktio_is_promisc >> >> Cheers, >> Anders >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h index e5cf320..87589ce 100644 --- a/platform/linux-generic/include/api/odp_packet_io.h +++ b/platform/linux-generic/include/api/odp_packet_io.h @@ -159,6 +159,31 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); int odp_pktio_mtu(odp_pktio_t id); /** + * Set promiscuous mode on a packet IO interface. + * + * @param[in] id ODP packet IO handle. + * @param[in] enable 1 to enable promiscuous mode, 0 to disable + * + * @retval 0 on success. + * @retval -1 on a bad pktio id + * @retval -1 ilegal enable value + * @retval -1 any other error + */ +int odp_pktio_set_promisc(odp_pktio_t id, int enable); + +/** + * Determine if promiscuous mode is enabled for a packet IO interface. + * + * @param[in] id ODP packet IO handle. + * + * @retval 1 if promiscuous mode is enabled. + * @retval 0 if promiscuous mode is disabled. + * @retval -1 on a bad pktio id + * @retval -1 any other error +*/ +int odp_pktio_promisc(odp_pktio_t id); + +/** * @} */ diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index f0e361a..403804c 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -542,3 +542,80 @@ int odp_pktio_mtu(odp_pktio_t id) return ifr.ifr_mtu; } + +int odp_pktio_set_promisc(odp_pktio_t id, int enable) +{ + pktio_entry_t *entry; + int sockfd; + struct ifreq ifr; + int ret; + + if (enable < 0 || enable > 1) { + ODP_DBG("illegal enable value %d\n", enable); + return -1; + } + + entry = get_entry(id); + if (entry == NULL) { + ODP_DBG("pktio entry %d does not exist\n", id); + return -1; + } + + if (entry->s.pkt_sock_mmap.sockfd) + sockfd = entry->s.pkt_sock_mmap.sockfd; + else + sockfd = entry->s.pkt_sock.sockfd; + + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ); + + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); + if (ret) { + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); + return -1; + } + + if (enable) + ifr.ifr_flags |= IFF_PROMISC; + else + ifr.ifr_flags &= ~(IFF_PROMISC); + + ret = ioctl(sockfd, SIOCSIFFLAGS, &ifr); + if (ret) { + ODP_DBG("ioctl SIOCSIFFLAGS error\n"); + return -1; + } + + return 0; +} + +int odp_pktio_promisc(odp_pktio_t id) +{ + pktio_entry_t *entry; + int sockfd; + struct ifreq ifr; + int ret; + + entry = get_entry(id); + if (entry == NULL) { + ODP_DBG("pktio entry %d does not exist\n", id); + return -1; + } + + if (entry->s.pkt_sock_mmap.sockfd) + sockfd = entry->s.pkt_sock_mmap.sockfd; + else + sockfd = entry->s.pkt_sock.sockfd; + + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ); + + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); + if (ret) { + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); + return -1; + } + + if (ifr.ifr_flags & IFF_PROMISC) + return 1; + else + return 0; +}
Define API and implement promisc functions: odp_pktio_set_promisc() odp_pktio_promisc() Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/api/odp_packet_io.h | 25 +++++++ platform/linux-generic/odp_packet_io.c | 77 ++++++++++++++++++++++ 2 files changed, 102 insertions(+)