On 27 November 2014 at 16:36, Ciprian Barbu <ciprian.ba...@linaro.org> wrote:
> On Thu, Nov 27, 2014 at 3:40 PM, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
>> On 11/27/2014 04:28 PM, Savolainen, Petri (NSN - FI/Espoo) wrote:
>>>
>>> Hi,
>>>
>>> I think this is what we talked on the call yesterday.
>>>
>>> /**
>>> * 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 Address size written, 0 on failure
>>
>>
>> -1 and -2
>>
>>
>>> */
>>> size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr, size_t
>>> addr_size);
>>>
>>>
>>> -Petri
>>>
>>>
>>>
>>> From: lng-odp-boun...@lists.linaro.org
>>> [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of ext Bala Manoharan
>>> Sent: Thursday, November 27, 2014 10:09 AM
>>> To: Maxim Uvarov
>>> Cc: lng-odp@lists.linaro.org
>>> Subject: Re: [lng-odp] [PATCH 3/5] pktio: mac addr functions
>>>
>>> Hi,
>>>
>>> I agree with Victor's concern, implementation needs a mechanism to know
>>> what is the amount of valid memory available in the mac_addr pointer.
>>> If I am not wrong the idea of defining the ODP_PKTIO_MAC_ADDR_MAX_LEN was
>>> initially discussed but the same was dropped as there were concerns since
>>> this value was an implementation detail and should be left outside of the
>>> API definition.
>>>
>>> Maybe we can have the following definition for odp_pktio_mac_addr
>>>
>>> +/**
>>> + * Get the default MAC address of a packet IO interface.
>>> + *
>>> + * @param[in] id ODP packet IO handle.
>>> + * @param[in/out] addr_size Memory available for storage / Size of
>>> the MAC address
>>> + * @param[out] mac_addr Storage for MAC address of the packet IO
>>> interface.
>>> + *
>>> + * @retval -1 on any error.
>>> + * @retval -2 if the specified storage in mac_addr is not enough
>>> + * @retval 0 on Success.
>>> + */
>>> +int odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_add, size_t
>>> *addr_size);
>>>
>>> The value addr_size could be an in/out parameter in the sense that the
>>> application while calling the API can specify memory available in the
>>> mac_addr pointer and the implementation can return the size of the mac
>>> address which gets filled.
>>>
>>> If the addr_size if smaller than the mac address of the interface the
>>> implementation can indicate the same using negative return value.
>>>
>
> You can also have a form similar to strncpy, where the return value
> specifies the number of bytes that were written or would have been
> needed:
Too complicated for this simple use case me thinks.
>
> /**
> * Get the default MAC address of a packet IO interface.
> *
> * @param[in] id ODP packet IO handle.
> * @param[in/out] addr_size Memory available for storage / Size
> of the MAC address
> * @param[out] mac_addr Storage for MAC address of the packet
> IO interface.
> * If set to NULL the function only returns the number of bytes
> needed to store the mac address.
> *
> * @retval -1 on error.
> * @retval greater than 0 specifying the size of the mac address.
> * If the value returned is bigger than addr_size then truncation
> occurs and the user
> * must call the function again.
> */
> size_t odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_add,
> size_t addr_size);
>>>
>>> Regards,
>>> Bala
>>>
>>>
>>> On 27 November 2014 at 04:05, Maxim Uvarov <maxim.uva...@linaro.org>
>>> wrote:
>>> On 11/27/2014 12:43 AM, Victor Kamensky wrote:
>>> On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uva...@linaro.org>
>>> wrote:
>>> On 11/26/2014 09:00 PM, Victor Kamensky wrote:
>>> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uva...@linaro.org>
>>> wrote:
>>> Define API for mac address change and implement linux-generic version.
>>>
>>> Signed-off-by: Maxim Uvarov <maxim.uva...@linaro.org>
>>> ---
>>> platform/linux-generic/include/api/odp_packet_io.h | 23 ++++++++
>>> platform/linux-generic/odp_packet_io.c | 67
>>> ++++++++++++++++++++++
>>> 2 files changed, 90 insertions(+)
>>>
>>> diff --git a/platform/linux-generic/include/api/odp_packet_io.h
>>> b/platform/linux-generic/include/api/odp_packet_io.h
>>> index 20425be..480d930 100644
>>> --- a/platform/linux-generic/include/api/odp_packet_io.h
>>> +++ b/platform/linux-generic/include/api/odp_packet_io.h
>>> @@ -173,6 +173,29 @@ int odp_pktio_promisc_set(odp_pktio_t id, odp_bool
>>> enable);
>>> int odp_pktio_promisc_enabled(odp_pktio_t id);
>>>
>>> /**
>>> + * Set the default MAC address of a packet IO interface.
>>> + *
>>> + * @param[in] id ODP packet IO handle.
>>> + * @param[in] mac_addr MAC address to be assigned to the interface.
>>> + * @param[in] addr_size Size of the address in bytes.
>>> + *
>>> + * @return 0 on success, -1 on error.
>>> + */
>>> +int odp_pktio_mac_addr_set(odp_pktio_t id, const unsigned char
>>> *mac_addr,
>>> + size_t addr_size);
>>> +
>>> +/**
>>> + * 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.
>>> How user knows what size should be allocated for this storage?
>>> Note if it is assumed to be fixed size, documentation should say
>>> so. But such approach would be inconsitent with odp_pktio_mac_addr_set
>>> function which does pass size of mac_addr storage to set.
>>>
>>> Maybe you want to pass size that user allocated
>>> for mac_addr storage and use it to copy result while
>>> returning real size of mac_addr, so user can compare
>>> whether it got all of it, and provide storage with required
>>> size if not.
>>>
>>> Thanks,
>>> Victor
>>>
>>> how about adding note that memory len for mac addr should use:
>>>
>>> #define ODP_PKTIO_MAC_ADDR_MAX_LEN 8
>>> ?
>>> It would not address my point about inconsistency between
>>> odp_pktio_mac_addr, and odp_pktio_mac_addr_set function.
>>> Why one has 'size' parameter, and another does not.
>>>
>>> Also if user always must past mac_addr pointer to storage
>>> size of ODP_PKTIO_MAC_ADDR_MAX_LEN, would it not be
>>> better to have its type as 'const unsigned
>>> char[ODP_PKTIO_MAC_ADDR_MAX]_LEN'?
>>>
>>> Thanks,
>>> Victor
>>>
>>> Understand your point now. Now I think that it's better for
>>> implementation to
>>> provide storage for mac address. It might be part of pktio_entry.
>>>
>>> So current call might be:
>>>
>>> int odp_pktio_mac_addr(odp_pktio_t id, unsigned char **mac_addr);
>>>
>>> Or even better we can provide to api hole pktio_enty struct.
>>>
>>> I.e.
>>>
>>> struct odp_pktio_entry * odp_pktio_get(id);
>>> And this return entry will have eveything for current pktio: name, mac,
>>> promisc mode and etc.
>>> So that we will have one function for everything. And it's will be linked
>>> to packet i/o handle. So we know
>>> when handle is freed all this data can not be accessed.
>>>
>>> But that might be too late for odp 1.0. And I don't want to delay with
>>> more round of review / discussion.
>>> Might be const unsigned char[ODP_PKTIO_MAC_ADDR_MAX_LEN] is good for now.
>>>
>>> Maxim.
>>>
>>>
>>> Maxim.
>>> + *
>>> + * @retval -1 on any error.
>>> + * @retval size of mac addr.
>>> + */
>>> +int odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr);
>>> +
>>> +/**
>>> * @}
>>> */
>>>
>>> diff --git a/platform/linux-generic/odp_packet_io.c
>>> b/platform/linux-generic/odp_packet_io.c
>>> index b1dbc41..72531b3 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];
>>> @@ -616,3 +617,69 @@ int odp_pktio_promisc_enabled(odp_pktio_t id)
>>> else
>>> return 0;
>>> }
>>> +
>>> +int odp_pktio_mac_addr_set(odp_pktio_t id, const unsigned char
>>> *mac_addr,
>>> + size_t addr_size)
>>> +{
>>> + 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 - 1);
>>> + ifr.ifr_name[IFNAMSIZ] = 0;
>>> + memcpy(ifr.ifr_hwaddr.sa_data, mac_addr, addr_size);
>>> + ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;
>>> +
>>> + ret = ioctl(sockfd, SIOCSIFHWADDR, &ifr);
>>> + if (ret < 0) {
>>> + ODP_DBG("ioctl SIOCSIFHWADDR error\n");
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int odp_pktio_mac_addr(odp_pktio_t id, unsigned char *mac_addr)
>>> +{
>>> + 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 - 1);
>>> + ifr.ifr_name[IFNAMSIZ] = 0;
>>> +
>>> + ret = ioctl(sockfd, SIOCGIFHWADDR, &ifr);
>>> + if (ret < 0) {
>>> + ODP_DBG("ioctl SIOCGIFHWADDR error\n");
>>> + return -1;
>>> + }
>>> +
>>> + 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
>>>
>>>
>>>
>>> _______________________________________________
>>> 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
>
> _______________________________________________
> 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
