On 11 November 2014 15:41, Bill Fischofer <[email protected]> wrote: > > > On Tue, Nov 11, 2014 at 2:28 PM, Anders Roxell <[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]> >> > --- >> > 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.
I think there are errors that can be listed in this case I don't think just saying -1 on error is enough for the documentation. I can't write unit tests for that, what cases am I testing ? Worse on non linux implementations you need to know exactly when to return error, saying errors match those thrown by linux ioctl is probably not accurate enough. we need @retval -1 requested MTU is not supported by the interface - if there is an errno that matters we need to list that too I guess > >> >> >> > + */ >> > +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. I agree with this, we don't want synonyms, a handle (hdl) should be a handle so that the ODP APIs all look and feel the same, currently we don't have this. >> >> >> > + * @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. > The doc says this http://docs.opendataplane.org/arch/html/api_guide_lines.html maybe there is something in this discussion we can capture there ? In this case I think it is less cumbersome to return the value you are looking for rather than have to pass a pointer to an int and populate that with the result, -1 could still be error, not sure that we disallow that in the doc (yet :) ) Unless we clearly always pass in a pointer for such things as part of our look and feel. I think Anders was eating spicy sausages when he put an int pointer however, it is int :) >> >> >> > + >> > /** >> > * @} >> > */ >> > 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. Checkpatch does a lot, but a lot can pass it that really jars viewers eyeballs. I completely agree however that to submit a patch it much be checkpatch clean, but I think there is scope for reviewers to ask for minor compatible changes for example to ask that lists line up, things like that. > >> >> >> 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, 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, 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, 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] >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> -- >> Anders Roxell >> [email protected] >> M: +46 709 71 42 85 | IRC: roxell >> >> _______________________________________________ >> 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 > -- Mike Holmes Linaro Sr Technical Manager LNG - ODP _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
