diff mbox

[PATCHv7,3/6] API: pktio: mac addr get function

Message ID 1418050713-29694-5-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Dec. 8, 2014, 2:58 p.m. UTC 
Define API to get MAC address for specific packet i/o and
implement linux-generic version.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Reviewed-by: Petri Savolainen <petri.savolainen@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(+)

Comments

Maxim Uvarov Dec. 9, 2014, 3:28 p.m. UTC | #1 
On 12/09/2014 06:04 PM, Stuart Haslam wrote:
> On Mon, Dec 08, 2014 at 02:58:30PM +0000, Maxim Uvarov wrote:
>> Define API to get MAC address for specific packet i/o and
>> implement linux-generic version.
>>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> Reviewed-by: Petri Savolainen <petri.savolainen@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 742ea59..63c047c 100644
>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>> @@ -175,6 +175,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	id	  ODP packet IO handle.
>> + * @param[out]	mac_addr  Storage for MAC address of the packet IO interface.
>> + * @param	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, void *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 7d2d2a4..e8815cd 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];
>> @@ -633,3 +634,40 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>>   	else
>>   		return 0;
>>   }
>> +
>> +size_t odp_pktio_mac_addr(odp_pktio_t id, void *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);
>> +	if (entry == NULL) {
>> +		ODP_DBG("pktio entry %d does not exist\n", id);
>> +		return 0;
>> +	}
>> +
>> +	lock_entry(entry);
>> +
> Why not just get the value from entry->s.pkt_sock.if_mac /
> entry->s.pkt_sock_mmap.if_mac?

In case if it will be changed. There was function to change mac. But we 
decided to postpone with it.
But some time later it will be there.

Maxim.

>> +	sockfd = sockfd_from_pktio_entry(entry);
>> +	strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
>> +	ifr.ifr_name[IFNAMSIZ - 1] = 0;
>> +
>> +	ret = ioctl(sockfd, SIOCGIFHWADDR, &ifr);
>> +	if (ret < 0) {
>> +		unlock_entry(entry);
>> +		ODP_DBG("ioctl SIOCGIFHWADDR error\n");
>> +		return 0;
>> +	}
>> +
>> +	memcpy(mac_addr, (unsigned char *)ifr.ifr_ifru.ifru_hwaddr.sa_data,
>> +	       ETH_ALEN);
>> +	unlock_entry(entry);
>> +
>> +	return ETH_ALEN;
>> +}
>> -- 
>> 1.8.5.1.163.gd7aced9
>>
>>
Maxim Uvarov Dec. 9, 2014, 3:35 p.m. UTC | #2 
On 12/09/2014 06:34 PM, Stuart Haslam wrote:
> On Tue, Dec 09, 2014 at 03:28:22PM +0000, Maxim Uvarov wrote:
>> On 12/09/2014 06:04 PM, Stuart Haslam wrote:
>>> On Mon, Dec 08, 2014 at 02:58:30PM +0000, Maxim Uvarov wrote:
>>>> Define API to get MAC address for specific packet i/o and
>>>> implement linux-generic version.
>>>>
>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>> Reviewed-by: Petri Savolainen <petri.savolainen@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 742ea59..63c047c 100644
>>>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>>>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>>>> @@ -175,6 +175,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	id	  ODP packet IO handle.
>>>> + * @param[out]	mac_addr  Storage for MAC address of the packet IO interface.
>>>> + * @param	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, void *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 7d2d2a4..e8815cd 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];
>>>> @@ -633,3 +634,40 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>>>>    	else
>>>>    		return 0;
>>>>    }
>>>> +
>>>> +size_t odp_pktio_mac_addr(odp_pktio_t id, void *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);
>>>> +	if (entry == NULL) {
>>>> +		ODP_DBG("pktio entry %d does not exist\n", id);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	lock_entry(entry);
>>>> +
>>> Why not just get the value from entry->s.pkt_sock.if_mac /
>>> entry->s.pkt_sock_mmap.if_mac?
>> In case if it will be changed. There was function to change mac. But we
>> decided to postpone with it.
>> But some time later it will be there.
>>
>> Maxim.
>>
> But in that case when setting the MAC you'd also need to update the stored
> pkt_sock.if_mac, as the incoming packet filtering is based on that being correct.
>
Yes, or remove it from there. I think that remove it better.

Maxim.
Maxim Uvarov Dec. 9, 2014, 3:56 p.m. UTC | #3 
On 12/09/2014 06:49 PM, Stuart Haslam wrote:
> On Tue, Dec 09, 2014 at 03:35:55PM +0000, Maxim Uvarov wrote:
>> On 12/09/2014 06:34 PM, Stuart Haslam wrote:
>>> On Tue, Dec 09, 2014 at 03:28:22PM +0000, Maxim Uvarov wrote:
>>>> On 12/09/2014 06:04 PM, Stuart Haslam wrote:
>>>>> On Mon, Dec 08, 2014 at 02:58:30PM +0000, Maxim Uvarov wrote:
>>>>>> Define API to get MAC address for specific packet i/o and
>>>>>> implement linux-generic version.
>>>>>>
>>>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>>>> Reviewed-by: Petri Savolainen <petri.savolainen@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 742ea59..63c047c 100644
>>>>>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>>>>>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>>>>>> @@ -175,6 +175,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	id	  ODP packet IO handle.
>>>>>> + * @param[out]	mac_addr  Storage for MAC address of the packet IO interface.
>>>>>> + * @param	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, void *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 7d2d2a4..e8815cd 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];
>>>>>> @@ -633,3 +634,40 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>>>>>>     	else
>>>>>>     		return 0;
>>>>>>     }
>>>>>> +
>>>>>> +size_t odp_pktio_mac_addr(odp_pktio_t id, void *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);
>>>>>> +	if (entry == NULL) {
>>>>>> +		ODP_DBG("pktio entry %d does not exist\n", id);
>>>>>> +		return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	lock_entry(entry);
>>>>>> +
>>>>> Why not just get the value from entry->s.pkt_sock.if_mac /
>>>>> entry->s.pkt_sock_mmap.if_mac?
>>>> In case if it will be changed. There was function to change mac. But we
>>>> decided to postpone with it.
>>>> But some time later it will be there.
>>>>
>>>> Maxim.
>>>>
>>> But in that case when setting the MAC you'd also need to update the stored
>>> pkt_sock.if_mac, as the incoming packet filtering is based on that being correct.
>>>
>> Yes, or remove it from there. I think that remove it better.
>>
>> Maxim.
>>
> The MAC address is stored in order to filter out packets sent by ourselves;
>
> https://git.linaro.org/lng/odp.git/blob/HEAD:/platform/linux-generic/odp_packet_socket.c#l451
> https://git.linaro.org/lng/odp.git/blob/HEAD:/platform/linux-generic/odp_packet_socket.c#l598
>
> Why would you want to change that behaviour?
>
Yes, agree now. Forgot about that filter. But in that case we should say 
that other app or other instance of odp can not change mac.
How do you think it's better update this value on get, set or both?


Maxim.
diff mbox

Patch

diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
index 742ea59..63c047c 100644
--- a/platform/linux-generic/include/api/odp_packet_io.h
+++ b/platform/linux-generic/include/api/odp_packet_io.h
@@ -175,6 +175,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	id	  ODP packet IO handle.
+ * @param[out]	mac_addr  Storage for MAC address of the packet IO interface.
+ * @param	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, void *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 7d2d2a4..e8815cd 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];
@@ -633,3 +634,40 @@  int odp_pktio_promisc_mode(odp_pktio_t id)
 	else
 		return 0;
 }
+
+size_t odp_pktio_mac_addr(odp_pktio_t id, void *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);
+	if (entry == NULL) {
+		ODP_DBG("pktio entry %d does not exist\n", id);
+		return 0;
+	}
+
+	lock_entry(entry);
+
+	sockfd = sockfd_from_pktio_entry(entry);
+	strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1);
+	ifr.ifr_name[IFNAMSIZ - 1] = 0;
+
+	ret = ioctl(sockfd, SIOCGIFHWADDR, &ifr);
+	if (ret < 0) {
+		unlock_entry(entry);
+		ODP_DBG("ioctl SIOCGIFHWADDR error\n");
+		return 0;
+	}
+
+	memcpy(mac_addr, (unsigned char *)ifr.ifr_ifru.ifru_hwaddr.sa_data,
+	       ETH_ALEN);
+	unlock_entry(entry);
+
+	return ETH_ALEN;
+}