| Message ID | 1417023121-19867-3-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > 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 | 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 > + * > + * @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
On 11/26/2014 09:00 PM, Victor Kamensky wrote: > On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> 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 | 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 ? 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
On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/26/2014 09:00 PM, Victor Kamensky wrote: >> >> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >>> >>> 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 | 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 > > 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 > >
On 11/27/2014 12:43 AM, Victor Kamensky wrote: > On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 11/26/2014 09:00 PM, Victor Kamensky wrote: >>> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>>> 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 | 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 >>
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. Regards, Bala On 27 November 2014 at 04:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/27/2014 12:43 AM, Victor Kamensky wrote: > >> On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> >>> On 11/26/2014 09:00 PM, Victor Kamensky wrote: >>> >>>> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> >>>> wrote: >>>> >>>>> 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 | 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 >
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-bounces@lists.linaro.org [mailto:lng-odp-bounces@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. > > > Regards, > Bala > > > On 27 November 2014 at 04:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/27/2014 12:43 AM, Victor Kamensky wrote: > On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/26/2014 09:00 PM, Victor Kamensky wrote: > On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > 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 | 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 >
On Thu, Nov 27, 2014 at 3:40 PM, Maxim Uvarov <maxim.uvarov@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-bounces@lists.linaro.org >> [mailto:lng-odp-bounces@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: /** * 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.uvarov@linaro.org> >> wrote: >> On 11/27/2014 12:43 AM, Victor Kamensky wrote: >> On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> On 11/26/2014 09:00 PM, Victor Kamensky wrote: >> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> 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 | 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
On 27 November 2014 at 16:36, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > On Thu, Nov 27, 2014 at 3:40 PM, Maxim Uvarov <maxim.uvarov@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-bounces@lists.linaro.org >>> [mailto:lng-odp-bounces@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.uvarov@linaro.org> >>> wrote: >>> On 11/27/2014 12:43 AM, Victor Kamensky wrote: >>> On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>> On 11/26/2014 09:00 PM, Victor Kamensky wrote: >>> On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>> 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 | 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
On 27 November 2014 at 14:28, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> 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 > */ > size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr, size_t addr_size); This is simple and should in practice cover all situations. MAC addresses are not of extremely variable size. In practice, only 48-bit and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used AFAIK. However I would rather return -1 on error (and use ssize_t as the return type). As a general convention I think we should use negative values for error and positive values for success. See e.g. POSIX read() call. -- Ola > > > -Petri > > > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-bounces@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. > > > Regards, > Bala > > > On 27 November 2014 at 04:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/27/2014 12:43 AM, Victor Kamensky wrote: > On 26 November 2014 at 13:19, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/26/2014 09:00 PM, Victor Kamensky wrote: > On 26 November 2014 at 09:31, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > 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 | 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
On 11/27/2014 06:48 PM, Ola Liljedahl wrote: > This is simple and should in practice cover all situations. MAC > addresses are not of extremely variable size. In practice, only 48-bit > and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used > AFAIK. Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and 64 bit macs? > However I would rather return -1 on error (and use ssize_t as the > return type). As a general convention I think we should use negative > values for error and positive values for success. See e.g. POSIX > read() call. > > -- Ola but size_t is unsigned. so that or it int or it's 0 on error, like Perti wrote. Maxim.
On 27 November 2014 at 17:10, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/27/2014 06:48 PM, Ola Liljedahl wrote: >> >> This is simple and should in practice cover all situations. MAC >> addresses are not of extremely variable size. In practice, only 48-bit >> and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used >> AFAIK. > > > Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and 64 bit macs? > >> However I would rather return -1 on error (and use ssize_t as the >> return type). As a general convention I think we should use negative >> values for error and positive values for success. See e.g. POSIX >> read() call. >> >> -- Ola > > > but size_t is unsigned. so that or it int or it's 0 on error, like Perti > wrote. That's why I referenced read(): ssize_t read(int fd, void *buf, size_t count); Uses ssize_t as return type so negative values can be returned. > > Maxim. > >
On 11/27/2014 07:20 PM, Ola Liljedahl wrote: > On 27 November 2014 at 17:10, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 11/27/2014 06:48 PM, Ola Liljedahl wrote: >>> This is simple and should in practice cover all situations. MAC >>> addresses are not of extremely variable size. In practice, only 48-bit >>> and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used >>> AFAIK. >> >> Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and 64 bit macs? >> >>> However I would rather return -1 on error (and use ssize_t as the >>> return type). As a general convention I think we should use negative >>> values for error and positive values for success. See e.g. POSIX >>> read() call. >>> >>> -- Ola >> >> but size_t is unsigned. so that or it int or it's 0 on error, like Perti >> wrote. > That's why I referenced read(): > ssize_t read(int fd, void *buf, size_t count); > > Uses ssize_t as return type so negative values can be returned. > > Interesting I did so. But not ssize_t, I used size_t and then on check if (-1 == ret) gcc errors that I'm comparing signed and unsigned. is ssize_t signed size? Maxim. >> Maxim. >> >>
On 27 November 2014 at 19:18, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/27/2014 07:20 PM, Ola Liljedahl wrote: >> >> On 27 November 2014 at 17:10, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >>> >>> On 11/27/2014 06:48 PM, Ola Liljedahl wrote: >>>> >>>> This is simple and should in practice cover all situations. MAC >>>> addresses are not of extremely variable size. In practice, only 48-bit >>>> and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used >>>> AFAIK. >>> >>> >>> Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and 64 bit >>> macs? >>> >>>> However I would rather return -1 on error (and use ssize_t as the >>>> return type). As a general convention I think we should use negative >>>> values for error and positive values for success. See e.g. POSIX >>>> read() call. >>>> >>>> -- Ola >>> >>> >>> but size_t is unsigned. so that or it int or it's 0 on error, like Perti >>> wrote. >> >> That's why I referenced read(): >> ssize_t read(int fd, void *buf, size_t count); >> >> Uses ssize_t as return type so negative values can be returned. >> >> > Interesting I did so. But not ssize_t, I used size_t and then on check if > (-1 == ret) > gcc errors that I'm comparing signed and unsigned. > > is ssize_t signed size? Right on I doubt we will be returning MAC addresses larger than would fit into ssize_t. > > > Maxim. > >>> Maxim. >>> >>> >
ssize_t (and size_t) are only 4 bytes on 32-bit systems, so they won't fit on those systems. For return codes what's the problem with simple a int as we've used elsewhere? Bill On Thu, Nov 27, 2014 at 3:18 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 27 November 2014 at 19:18, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > > On 11/27/2014 07:20 PM, Ola Liljedahl wrote: > >> > >> On 27 November 2014 at 17:10, Maxim Uvarov <maxim.uvarov@linaro.org> > >> wrote: > >>> > >>> On 11/27/2014 06:48 PM, Ola Liljedahl wrote: > >>>> > >>>> This is simple and should in practice cover all situations. MAC > >>>> addresses are not of extremely variable size. In practice, only 48-bit > >>>> and 64-bit MAC addresses (EUI - Extended Unique Identifier) are used > >>>> AFAIK. > >>> > >>> > >>> Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and 64 bit > >>> macs? > >>> > >>>> However I would rather return -1 on error (and use ssize_t as the > >>>> return type). As a general convention I think we should use negative > >>>> values for error and positive values for success. See e.g. POSIX > >>>> read() call. > >>>> > >>>> -- Ola > >>> > >>> > >>> but size_t is unsigned. so that or it int or it's 0 on error, like > Perti > >>> wrote. > >> > >> That's why I referenced read(): > >> ssize_t read(int fd, void *buf, size_t count); > >> > >> Uses ssize_t as return type so negative values can be returned. > >> > >> > > Interesting I did so. But not ssize_t, I used size_t and then on check if > > (-1 == ret) > > gcc errors that I'm comparing signed and unsigned. > > > > is ssize_t signed size? > Right on > > I doubt we will be returning MAC addresses larger than would fit into > ssize_t. > > > > > > > Maxim. > > > >>> Maxim. > >>> > >>> > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 11/28/2014 05:46 AM, Bill Fischofer wrote: > ssize_t (and size_t) are only 4 bytes on 32-bit systems, so they won't > fit on those systems. > Why they won't fit? Mac address is 6 or 8 bytes. Size can be coded even with 1 byte. > For return codes what's the problem with simple a int as we've used > elsewhere? > > Bill > > On Thu, Nov 27, 2014 at 3:18 PM, Ola Liljedahl > <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>> wrote: > > On 27 November 2014 at 19:18, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > On 11/27/2014 07:20 PM, Ola Liljedahl wrote: > >> > >> On 27 November 2014 at 17:10, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> > >> wrote: > >>> > >>> On 11/27/2014 06:48 PM, Ola Liljedahl wrote: > >>>> > >>>> This is simple and should in practice cover all situations. MAC > >>>> addresses are not of extremely variable size. In practice, > only 48-bit > >>>> and 64-bit MAC addresses (EUI - Extended Unique Identifier) > are used > >>>> AFAIK. > >>> > >>> > >>> Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and > 64 bit > >>> macs? > >>> > >>>> However I would rather return -1 on error (and use ssize_t as the > >>>> return type). As a general convention I think we should use > negative > >>>> values for error and positive values for success. See e.g. POSIX > >>>> read() call. > >>>> > >>>> -- Ola > >>> > >>> > >>> but size_t is unsigned. so that or it int or it's 0 on error, > like Perti > >>> wrote. > >> > >> That's why I referenced read(): > >> ssize_t read(int fd, void *buf, size_t count); > >> > >> Uses ssize_t as return type so negative values can be returned. > >> > >> > > Interesting I did so. But not ssize_t, I used size_t and then on > check if > > (-1 == ret) > > gcc errors that I'm comparing signed and unsigned. > > > > is ssize_t signed size? > Right on > > I doubt we will be returning MAC addresses larger than would fit > into ssize_t. > > > > > > > Maxim. > > > >>> Maxim. > >>> > >>> > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
Sorry, I misread that you were returning the MAC address in a ssize_t. The length, of course, would be fine for that. On Fri, Nov 28, 2014 at 4:25 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/28/2014 05:46 AM, Bill Fischofer wrote: > >> ssize_t (and size_t) are only 4 bytes on 32-bit systems, so they won't >> fit on those systems. >> >> > Why they won't fit? Mac address is 6 or 8 bytes. Size can be coded even > with 1 byte. > > > For return codes what's the problem with simple a int as we've used >> elsewhere? >> >> Bill >> >> On Thu, Nov 27, 2014 at 3:18 PM, Ola Liljedahl <ola.liljedahl@linaro.org >> <mailto:ola.liljedahl@linaro.org>> wrote: >> >> On 27 November 2014 at 19:18, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> > On 11/27/2014 07:20 PM, Ola Liljedahl wrote: >> >> >> >> On 27 November 2014 at 17:10, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> >> >> >> wrote: >> >>> >> >>> On 11/27/2014 06:48 PM, Ola Liljedahl wrote: >> >>>> >> >>>> This is simple and should in practice cover all situations. MAC >> >>>> addresses are not of extremely variable size. In practice, >> only 48-bit >> >>>> and 64-bit MAC addresses (EUI - Extended Unique Identifier) >> are used >> >>>> AFAIK. >> >>> >> >>> >> >>> Can linux on ioctl(sockfd, SIOCSIFHWADDR, ..) use both 48 and >> 64 bit >> >>> macs? >> >>> >> >>>> However I would rather return -1 on error (and use ssize_t as the >> >>>> return type). As a general convention I think we should use >> negative >> >>>> values for error and positive values for success. See e.g. POSIX >> >>>> read() call. >> >>>> >> >>>> -- Ola >> >>> >> >>> >> >>> but size_t is unsigned. so that or it int or it's 0 on error, >> like Perti >> >>> wrote. >> >> >> >> That's why I referenced read(): >> >> ssize_t read(int fd, void *buf, size_t count); >> >> >> >> Uses ssize_t as return type so negative values can be returned. >> >> >> >> >> > Interesting I did so. But not ssize_t, I used size_t and then on >> check if >> > (-1 == ret) >> > gcc errors that I'm comparing signed and unsigned. >> > >> > is ssize_t signed size? >> Right on >> >> I doubt we will be returning MAC addresses larger than would fit >> into ssize_t. >> >> > >> > >> > Maxim. >> > >> >>> Maxim. >> >>> >> >>> >> > >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
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. + * + * @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; +}
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 | 23 ++++++++ platform/linux-generic/odp_packet_io.c | 67 ++++++++++++++++++++++ 2 files changed, 90 insertions(+)