On 27 November 2014 at 14:28, Savolainen, Petri (NSN - FI/Espoo) <[email protected]> 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: [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. > > > 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
