[PATCHv4,3/6] API: pktio: mac addr functions

Define API for mac address change and implement linux-generic version.

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

On 3 December 2014 at 07:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> Define API for mac address change and implement linux-generic version.

In above comment 'change' is used. I don't see code that
changes mac address in below diff. Change 'change' to 'read'.

>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/include/api/odp_packet_io.h | 12 +++++++
>  platform/linux-generic/odp_packet_io.c             | 38 ++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
> index df2b138..152d23b 100644
> --- a/platform/linux-generic/include/api/odp_packet_io.h
> +++ b/platform/linux-generic/include/api/odp_packet_io.h
> @@ -177,6 +177,18 @@ int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t enable);
>  int odp_pktio_promisc_mode(odp_pktio_t id);
>
>  /**
> + * Get the default MAC address of a packet IO interface.
> + *
> + * @param[in]  id         ODP packet IO handle.
> + * @param[out] mac_addr   Storage for MAC address of the packet IO interface.
> + * @param[in]  addr_size  Storage size for the address
> + *
> + * @retval Number of bytes written on success, 0 on failure.
> + */
> +size_t odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr,
> +                         size_t addr_size);
> +
> +/**
>   * @}
>   */
>
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index 8286728..595ba56 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -21,6 +21,7 @@
>
>  #include <string.h>
>  #include <sys/ioctl.h>
> +#include <linux/if_arp.h>
>
>  typedef struct {
>         pktio_entry_t entries[ODP_CONFIG_PKTIO_ENTRIES];
> @@ -623,3 +624,40 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>         else
>                 return 0;
>  }
> +
> +size_t odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr,
> +                      size_t addr_size)
> +{
> +       pktio_entry_t *entry;
> +       int sockfd;
> +       struct ifreq ifr;
> +       int ret;
> +
> +       if (addr_size < ETH_ALEN)
> +               return 0;
> +
> +       entry = get_entry(id);

As in another patch. It seems that lock_entry is missing.

> +       if (entry == NULL) {
> +               ODP_DBG("pktio entry %d does not exist\n", id);
> +               return 0;
> +       }
> +
> +       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;

Again, as in previous patch comment above snippet is
replicated again and again. Maybe you need to create
helper function that retrieves entry, set sockfd, and fill ifr
structure for further processing.

Thanks,
Victor

> +
> +       ret = ioctl(sockfd, SIOCGIFHWADDR, &ifr);
> +       if (ret < 0) {
> +               ODP_DBG("ioctl SIOCGIFHWADDR error\n");
> +               return 0;
> +       }
> +
> +       memcpy(mac_addr, (unsigned char *)ifr.ifr_ifru.ifru_hwaddr.sa_data,
> +              ETH_ALEN);
> +
> +       return ETH_ALEN;
> +}
> --
> 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 02:06 AM, Victor Kamensky wrote:
> On 3 December 2014 at 07:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>> Define API for mac address change and implement linux-generic version.
> In above comment 'change' is used. I don't see code that
> changes mac address in below diff. Change 'change' to 'read'.
>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   platform/linux-generic/include/api/odp_packet_io.h | 12 +++++++
>>   platform/linux-generic/odp_packet_io.c             | 38 ++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
>> index df2b138..152d23b 100644
>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>> @@ -177,6 +177,18 @@ int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t enable);
>>   int odp_pktio_promisc_mode(odp_pktio_t id);
>>
>>   /**
>> + * Get the default MAC address of a packet IO interface.
>> + *
>> + * @param[in]  id         ODP packet IO handle.
>> + * @param[out] mac_addr   Storage for MAC address of the packet IO interface.
>> + * @param[in]  addr_size  Storage size for the address
>> + *
>> + * @retval Number of bytes written on success, 0 on failure.
>> + */
>> +size_t odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr,
>> +                         size_t addr_size);
>> +
>> +/**
>>    * @}
>>    */
>>
>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
>> index 8286728..595ba56 100644
>> --- a/platform/linux-generic/odp_packet_io.c
>> +++ b/platform/linux-generic/odp_packet_io.c
>> @@ -21,6 +21,7 @@
>>
>>   #include <string.h>
>>   #include <sys/ioctl.h>
>> +#include <linux/if_arp.h>
>>
>>   typedef struct {
>>          pktio_entry_t entries[ODP_CONFIG_PKTIO_ENTRIES];
>> @@ -623,3 +624,40 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>>          else
>>                  return 0;
>>   }
>> +
>> +size_t odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr,
>> +                      size_t addr_size)
>> +{
>> +       pktio_entry_t *entry;
>> +       int sockfd;
>> +       struct ifreq ifr;
>> +       int ret;
>> +
>> +       if (addr_size < ETH_ALEN)
>> +               return 0;
>> +
>> +       entry = get_entry(id);
> As in another patch. It seems that lock_entry is missing.
>
>> +       if (entry == NULL) {
>> +               ODP_DBG("pktio entry %d does not exist\n", id);
>> +               return 0;
>> +       }
>> +
>> +       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;
> Again, as in previous patch comment above snippet is
> replicated again and again. Maybe you need to create
> helper function that retrieves entry, set sockfd, and fill ifr
> structure for further processing.
>
> Thanks,
> Victor

did set socket fd in helper. But ifr and names .... current code is not 
so big and it's clear without helpers. I don't think it will
look good.

Maxim.

>
>> +
>> +       ret = ioctl(sockfd, SIOCGIFHWADDR, &ifr);
>> +       if (ret < 0) {
>> +               ODP_DBG("ioctl SIOCGIFHWADDR error\n");
>> +               return 0;
>> +       }
>> +
>> +       memcpy(mac_addr, (unsigned char *)ifr.ifr_ifru.ifru_hwaddr.sa_data,
>> +              ETH_ALEN);
>> +
>> +       return ETH_ALEN;
>> +}
>> --
>> 1.8.5.1.163.gd7aced9
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp