[PATCHv4,2/6] API: promisc mode manipulation functions

Define API and implement promisc functions for linux-generic.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++
 platform/linux-generic/odp_packet_io.c             | 74 ++++++++++++++++++++++
 2 files changed, 98 insertions(+)

On 3 December 2014 at 07:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> Define API and implement promisc functions for linux-generic.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++
>  platform/linux-generic/odp_packet_io.c             | 74 ++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
>
> diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
> index 5f7ba7c..df2b138 100644
> --- a/platform/linux-generic/include/api/odp_packet_io.h
> +++ b/platform/linux-generic/include/api/odp_packet_io.h
> @@ -153,6 +153,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu);
>  int odp_pktio_mtu(odp_pktio_t id);
>
>  /**
> + * Enable promiscuous mode on a packet IO interface.
> + *
> + * @param[in] id       ODP packet IO handle.
> + * @param[in] enable    1 enabled, 0 disabled.
> + *
> + * @retval  0 on success.
> + * @retval -1 on a bad pktio id
> + * @retval -1 any other error
> + */
> +int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t 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_mode(odp_pktio_t id);
> +
> +/**
>   * @}
>   */
>
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index faa197f..8286728 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -549,3 +549,77 @@ int odp_pktio_mtu(odp_pktio_t id)
>
>         return ifr.ifr_mtu;
>  }
> +
> +int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t enable)
> +{
> +       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;
> +       }

Don't you need to lock entry? It seems that all
other functions in odp_packet_io.c do that:

lock_entry(entry);

Consider situation that some other thread does
close on this pktio handle.

Of course you would need to unlock it latter at
proper point(s). You may need to restructure
code to avoid that many returns.

> +
> +       if (entry->s.pkt_sock_mmap.sockfd > -1)
> +               sockfd = entry->s.pkt_sock_mmap.sockfd;
> +       else
> +               sockfd = entry->s.pkt_sock.sockfd;
> +
> +       strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
> +       ifr.ifr_name[IFNAMSIZ - 1] = 0;
> +
> +       ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr);
> +       if (ret < 0) {
> +               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 < 0) {
> +               ODP_DBG("ioctl SIOCSIFFLAGS error\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +int odp_pktio_promisc_mode(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 > -1)
> +               sockfd = entry->s.pkt_sock_mmap.sockfd;
> +       else
> +               sockfd = entry->s.pkt_sock.sockfd;
> +
> +       strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
> +       ifr.ifr_name[IFNAMSIZ - 1] = 0;
> +
> +       ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr);
> +       if (ret < 0) {
> +               ODP_DBG("ioctl SIOCGIFFLAGS error\n");
> +               return -1;
> +       }

What bothers me with above two functions is big amount of
overlapping code between odp_pktio_promisc_mode and
odp_pktio_promisc_mode_set.

Wondering whether it is possible to come up with common
static function that does both set and get and such function
would be called from public APIs.

Thanks,
Victor

> +
> +       if (ifr.ifr_flags & IFF_PROMISC)
> +               return 1;
> +       else
> +               return 0;
> +}
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

On 12/04/2014 01:49 AM, Victor Kamensky wrote:
> On 3 December 2014 at 07:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>> Define API and implement promisc functions for linux-generic.
>>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++
>>   platform/linux-generic/odp_packet_io.c             | 74 ++++++++++++++++++++++
>>   2 files changed, 98 insertions(+)
>>
>> diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
>> index 5f7ba7c..df2b138 100644
>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>> @@ -153,6 +153,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu);
>>   int odp_pktio_mtu(odp_pktio_t id);
>>
>>   /**
>> + * Enable promiscuous mode on a packet IO interface.
>> + *
>> + * @param[in] id       ODP packet IO handle.
>> + * @param[in] enable    1 enabled, 0 disabled.
>> + *
>> + * @retval  0 on success.
>> + * @retval -1 on a bad pktio id
>> + * @retval -1 any other error
>> + */
>> +int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t 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_mode(odp_pktio_t id);
>> +
>> +/**
>>    * @}
>>    */
>>
>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
>> index faa197f..8286728 100644
>> --- a/platform/linux-generic/odp_packet_io.c
>> +++ b/platform/linux-generic/odp_packet_io.c
>> @@ -549,3 +549,77 @@ int odp_pktio_mtu(odp_pktio_t id)
>>
>>          return ifr.ifr_mtu;
>>   }
>> +
>> +int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t enable)
>> +{
>> +       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;
>> +       }
> Don't you need to lock entry? It seems that all
> other functions in odp_packet_io.c do that:
>
> lock_entry(entry);
>
> Consider situation that some other thread does
> close on this pktio handle.
>
> Of course you would need to unlock it latter at
> proper point(s). You may need to restructure
> code to avoid that many returns.

Yes, I thought about if I need locks or I don't. And until we do not 
close existence pktio then
we don't need looks but if we close then we need them. So will add.

Maxim.


>
>> +
>> +       if (entry->s.pkt_sock_mmap.sockfd > -1)
>> +               sockfd = entry->s.pkt_sock_mmap.sockfd;
>> +       else
>> +               sockfd = entry->s.pkt_sock.sockfd;
>> +
>> +       strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
>> +       ifr.ifr_name[IFNAMSIZ - 1] = 0;
>> +
>> +       ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr);
>> +       if (ret < 0) {
>> +               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 < 0) {
>> +               ODP_DBG("ioctl SIOCSIFFLAGS error\n");
>> +               return -1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +int odp_pktio_promisc_mode(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 > -1)
>> +               sockfd = entry->s.pkt_sock_mmap.sockfd;
>> +       else
>> +               sockfd = entry->s.pkt_sock.sockfd;
>> +
>> +       strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
>> +       ifr.ifr_name[IFNAMSIZ - 1] = 0;
>> +
>> +       ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr);
>> +       if (ret < 0) {
>> +               ODP_DBG("ioctl SIOCGIFFLAGS error\n");
>> +               return -1;
>> +       }
> What bothers me with above two functions is big amount of
> overlapping code between odp_pktio_promisc_mode and
> odp_pktio_promisc_mode_set.
>
> Wondering whether it is possible to come up with common
> static function that does both set and get and such function
> would be called from public APIs.
>
> Thanks,
> Victor

I put socket fd selection to static function. So difference is only one 
ioctl. That might be
more readable then move it so separate function.

Maxim.

>
>> +
>> +       if (ifr.ifr_flags & IFF_PROMISC)
>> +               return 1;
>> +       else
>> +               return 0;
>> +}
>> --
>> 1.8.5.1.163.gd7aced9
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp