On Thu, Nov 27, 2014 at 3:40 PM, Maxim Uvarov <[email protected]> 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: [email protected] >> [mailto:[email protected]] On Behalf Of ext Bala Manoharan >> Sent: Thursday, November 27, 2014 10:09 AM >> To: Maxim Uvarov >> Cc: [email protected] >> 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: /** * 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 <[email protected]> >> wrote: >> On 11/27/2014 12:43 AM, Victor Kamensky wrote: >> On 26 November 2014 at 13:19, Maxim Uvarov <[email protected]> >> wrote: >> On 11/26/2014 09:00 PM, Victor Kamensky wrote: >> On 26 November 2014 at 09:31, Maxim Uvarov <[email protected]> >> wrote: >> Define API for mac address change and implement linux-generic version. >> >> Signed-off-by: Maxim Uvarov <[email protected]> >> --- >> 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 >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
