On 12 November 2014 07:54, Maxim Uvarov <[email protected]> wrote: > I wonder why these question were risen now. In patch review, not in arch > review :) > But summarizing everything: > > 1. Patches are checkpatch clean. > 2. Minor style check for comment - agree will correct it. > 3. Requested change 'odp_pktio_t id' to 'odp_pktio_t hdl' is not correct. > That is subject for other patch. > Code has to look same for all function. Everywhere we use 'odp_pktio_t id'. > Run: fgrep -r odp_pktio_t ./platform/linux-generic > 4. Return codes. I followed return codes as API proposed.
Yes agreed and universally the documentation has not defined the error cases making meaningful testing of the 1.0 spec virtually impossible. <- my key point - my rational follows Given that, we must add information to the arch docs as you say below, the correct location is the definition of this API in its header file. The arch docs are stale as soon as an API becomes code that is why when it is built, the arch doc <http://docs.opendataplane.org/arch/html/group__odp__packet__io.html> links to the API docs for this information. In this case there is no arch doc for this API so we need to just add it to the best of our ability in the header file and it will become part of the arch doc and its specification will be testable, linux-generic is is primarily the reference for odp 1.0 not a performance platform. With this implementation you have added an obvious testable error case, if we don't specify it ODP becomes a specification with undefined return behavior. I agree some platforms may have additional error cases (we may add them as they turn up) but an implementer may wish to rely on the defined behavior, consider a contrived case of the general problem // find biggest MTU that is working for this packet_io mtu =0 mtu_status = odp_pktio_set_mtu( id, mtu) while (mtu_status != ok) { mtu++; mtu_status = odp_pktio_set_mtu( id, mtu) } This is not possible for an api that doesn't specify what a bad MTU value will do, it may call abort! Compare our docs for this function and an admittedly more complex linux read - the open group list what will cause an error, they describe how an API is actually supposed to react http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html. Please note that I > used ioctl, but I could do it in other way. > So it is only implementation. Inside implementation function I added > ODP_ERR. If you need specific reason > run strace on your application, that is very common for linux apps. > If we decided to add more error codes to odp, then we can add. Like > ERROR_MTU_TOO_SMALL, ERROR_MTU_TOO_BIG. > But that has to go to arch first. And I would like to have it in separate > patch. To discuss with vendors if that returns codes > are valuable and if they can really return them. > > Maxim. > > On 11/11/2014 11:41 PM, Bill Fischofer wrote: >> >> >> >> On Tue, Nov 11, 2014 at 2:28 PM, Anders Roxell <[email protected] >> <mailto:[email protected]>> wrote: >> >> Drop linux-generic because we only have linux-generic and say >> something >> like: >> "pktio: add MTU manipulation functions" >> >> On 2014-11-11 21:14, Maxim Uvarov wrote: >> > Implement pktio mtu functions: >> > odp_pktio_mtu() to get mtu value; >> > odp_pktio_set_mtu() to set mtu value. >> > >> > Signed-off-by: Maxim Uvarov <[email protected] >> <mailto:[email protected]>> >> >> > --- >> > platform/linux-generic/include/api/odp_packet_io.h | 20 +++++++++ >> > .../linux-generic/include/odp_packet_io_internal.h | 4 ++ >> > platform/linux-generic/odp_packet_io.c | 50 ++++++++++++++++++++++ >> > 3 files changed, 74 insertions(+) >> > >> > diff --git a/platform/linux-generic/include/api/odp_packet_io.h >> b/platform/linux-generic/include/api/odp_packet_io.h >> > index 360636d..b784705 100644 >> > --- a/platform/linux-generic/include/api/odp_packet_io.h >> > +++ b/platform/linux-generic/include/api/odp_packet_io.h >> > @@ -135,6 +135,26 @@ void odp_pktio_set_input(odp_packet_t pkt, >> odp_pktio_t id); >> > */ >> > odp_pktio_t odp_pktio_get_input(odp_packet_t pkt); >> > >> > +/* >> > + * Configure the MTU for a packet IO interface. >> >> Is this description enough for a new user to understand >> what will occur? >> >> >> I'd think so. MTU is a pretty standard bit of interface metadata. >> >> >> > + * >> > + * @param[in] id ODP packet IO handle. >> >> hdl should be the standard way for ODP to specify the handle. >> >> > + * @param[in] mtu The MTU to be applied. >> >> The value of MTU that the interface will be configured to use. >> >> > + * >> > + * @return 0 on success, -1 on error. >> >> @retval 0 <reason> >> @retval -1 <reason(s)> >> >> Elaborate on what causes an error, if it's not possible to provide a >> good reason for -1 should this be a void return? >> >> >> The ioctl may fail. Wouldn't the application want to know that? For >> example the range of valid MTUs will vary by device and ODP can't be >> expected to know that. It just calls the ioctl and Linux tells whether >> that's OK. Since linux will set errno on ioctl failure, -1 is just >> following the standard ODP error return protocol. >> >> >> > + */ >> > +int odp_pktio_set_mtu(odp_pktio_t id, unsigned int mtu); >> > + >> > +/* >> > + * Get the currently configured MTU of a packet IO interface. >> >> Is this description enough for a new user to understand >> what will occur? >> >> >> Again, yes. >> >> >> > + * >> > + * @param[in] id ODP packet IO handle. >> >> Does not line up >> >> hdl should be the standard way for ODP to specify the handle. >> >> >> > + * @param[out] mtu Pointer to location in which to store the >> MTU value. >> > + * >> > + * @return 0 on success, -1 on error. >> > + */ >> > +int odp_pktio_mtu(odp_pktio_t id, unsigned int *mtu); >> >> unsigned int *odp_pktio_mtu(odp_pktio_t id); >> >> @retval mtu on succes >> @retval NULL on error <list all the errors> >> >> >> No, this routine has an output parameter. This is just following standard >> ODP RC conventions for reporting success/failure as an int function return >> value. NULL can only be assigned to a pointer, not an int. Regarding the >> errors, again the ioctl will set errno so if the caller want's to know why >> it failed it can read that. It's system-dependent since each interface may >> be slightly different. >> >> >> > + >> > /** >> > * @} >> > */ >> > diff --git >> a/platform/linux-generic/include/odp_packet_io_internal.h >> b/platform/linux-generic/include/odp_packet_io_internal.h >> > index 23633ed..ff73970 100644 >> > --- a/platform/linux-generic/include/odp_packet_io_internal.h >> > +++ b/platform/linux-generic/include/odp_packet_io_internal.h >> > @@ -21,6 +21,8 @@ extern "C" { >> > #include <odp_spinlock.h> >> > #include <odp_packet_socket.h> >> > >> > +#include <linux/if.h> >> > + >> > /** >> > * Packet IO types >> > */ >> > @@ -38,6 +40,8 @@ struct pktio_entry { >> > odp_pktio_type_t type; /**< pktio type */ >> > pkt_sock_t pkt_sock; /**< using socket API for >> IO */ >> > pkt_sock_mmap_t pkt_sock_mmap; /**< using socket mmap API >> for IO */ >> > + char name[IFNAMSIZ]; /**< name of pktio provided to >> > + pktio_open() */ >> >> Does not line up. >> >> >> Doesn't checkpatch catch actual style errors? I'm assuming this is >> checkpatch clean. If it is that should be sufficient. If it's not, then it >> should be made checkpatch-clean before posting it. >> >> >> Cheers, >> Anders >> >> > }; >> > >> > typedef union { >> > diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> > index f35193f..20fe10a 100644 >> > --- a/platform/linux-generic/odp_packet_io.c >> > +++ b/platform/linux-generic/odp_packet_io.c >> > @@ -20,6 +20,7 @@ >> > #include <odp_debug.h> >> > >> > #include <string.h> >> > +#include <sys/ioctl.h> >> > >> > typedef struct { >> > pktio_entry_t entries[ODP_CONFIG_PKTIO_ENTRIES]; >> > @@ -203,6 +204,7 @@ odp_pktio_t odp_pktio_open(const char *dev, >> odp_buffer_pool_t pool) >> > return ODP_PKTIO_INVALID; >> > >> > done: >> > + strncpy(pktio_entry->s.name <http://s.name>, dev, IFNAMSIZ); >> > unlock_entry(pktio_entry); >> > return id; >> > } >> > @@ -476,3 +478,51 @@ int pktin_deq_multi(queue_entry_t *qentry, >> odp_buffer_hdr_t *buf_hdr[], int num) >> > >> > return nbr; >> > } >> > + >> > +int odp_pktio_set_mtu(odp_pktio_t id, unsigned int mtu) >> > +{ >> > + pktio_entry_t *pktio_entry = get_entry(id); >> > + int sockfd; >> > + struct ifreq ifr; >> > + int ret; >> > + >> > + if (pktio_entry->s.pkt_sock_mmap.sockfd) >> > + sockfd = pktio_entry->s.pkt_sock_mmap.sockfd; >> > + else >> > + sockfd = pktio_entry->s.pkt_sock.sockfd; >> > + >> > + strncpy(ifr.ifr_name, pktio_entry->s.name <http://s.name>, >> IFNAMSIZ); >> > + ifr.ifr_mtu = mtu; >> > + >> > + ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr); >> > + if (ret != 0) { >> > + ODP_ERR("ioctl SIOCSIFMTU error\n"); >> > + return -1; >> > + } >> > + >> > + return 0; >> > +} >> > + >> > +int odp_pktio_mtu(odp_pktio_t id, unsigned int *mtu) >> > +{ >> > + pktio_entry_t *pktio_entry = get_entry(id); >> > + int sockfd; >> > + struct ifreq ifr; >> > + int ret; >> > + >> > + if (pktio_entry->s.pkt_sock_mmap.sockfd) >> > + sockfd = pktio_entry->s.pkt_sock_mmap.sockfd; >> > + else >> > + sockfd = pktio_entry->s.pkt_sock.sockfd; >> > + >> > + strncpy(ifr.ifr_name, pktio_entry->s.name <http://s.name>, >> IFNAMSIZ); >> > + >> > + ret = ioctl(sockfd, SIOCGIFMTU, &ifr); >> > + if (ret != 0) { >> > + ODP_ERR("ioctl SIOCGIFMTU error\n"); >> > + return -1; >> > + } >> > + >> > + *mtu = ifr.ifr_mtu; >> > + return 0; >> > +} >> > -- >> > 1.8.5.1.163.gd7aced9 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > [email protected] <mailto:[email protected]> >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> -- >> Anders Roxell >> [email protected] <mailto:[email protected]> >> M: +46 709 71 42 85 <tel:%2B46%20709%2071%2042%2085> | IRC: roxell >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp -- Mike Holmes Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
