diff mbox

[PATCHv4,1/3] pktio: add MTU manipulation functions

Message ID 1415957459-11458-2-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Nov. 14, 2014, 9:30 a.m. UTC
Implement pktio mtu functions:
odp_pktio_mtu() to get mtu value;
odp_pktio_set_mtu() to set mtu value.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/include/api/odp_packet_io.h | 23 ++++++++
 .../linux-generic/include/odp_packet_io_internal.h |  4 ++
 platform/linux-generic/odp_packet_io.c             | 66 ++++++++++++++++++++++
 3 files changed, 93 insertions(+)

Comments

Mike Holmes Nov. 14, 2014, 7:50 p.m. UTC | #1
On 14 November 2014 04:30, Maxim Uvarov <maxim.uvarov@linaro.org> 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 <maxim.uvarov@linaro.org>
>
> ---
>  platform/linux-generic/include/api/odp_packet_io.h | 23 ++++++++
>  .../linux-generic/include/odp_packet_io_internal.h |  4 ++
>  platform/linux-generic/odp_packet_io.c             | 66 ++++++++++++++++++++++
>  3 files changed, 93 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..e5cf320 100644
> --- a/platform/linux-generic/include/api/odp_packet_io.h
> +++ b/platform/linux-generic/include/api/odp_packet_io.h
> @@ -136,6 +136,29 @@ 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.
> + *
> + * @param[in] id   ODP packet IO handle.
> + * @param[in] mtu  The value of MTU that the interface will be configured to
> + *                use.
> + *
> + * @retval  0 on success.
> + * @retval -1 if specified mtu can not be handled.
> + * @retval -1 on any other error or illegal input parameters.
> + */
> +int odp_pktio_set_mtu(odp_pktio_t id, int mtu);
> +
> +/**
> + * Return the currently configured MTU value of a packet IO interface.
> + *
> + * @param[in] id  ODP packet IO handle.
> + *
> + * @retval MTU value >0 on success.
> + * @retval -1 on any error or not existance pktio id.
> + */
> +int odp_pktio_mtu(odp_pktio_t id);
> +
> +/**
>   * @}
>   */
>
> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> index 23633ed..0bc1e21 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() */
>  };
>
>  typedef union {
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index f35193f..f0e361a 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,67 @@ 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, int mtu)
> +{
> +       pktio_entry_t *entry;
> +       int sockfd;
> +       struct ifreq ifr;
> +       int ret;
> +
> +       if (mtu <= 0) {
> +               ODP_DBG("illegal MTU value %d\n", mtu);
> +               return -1;
> +       }
> +
> +       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);
> +       ifr.ifr_mtu = mtu;
> +
> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
> +       if (ret) {

From the ioctl man page
Usually, on success zero is returned.  A few ioctl() requests use the
       return value as an output parameter and return a nonnegative value on
       success.  On error, -1 is returned, and errno is set appropriately.

So in this case you need to check for (-1==ret)

>
> +               ODP_DBG("ioctl SIOCSIFMTU error\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +int odp_pktio_mtu(odp_pktio_t id)
> +{
> +       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);
> +
> +       ret = ioctl(sockfd, SIOCGIFMTU, &ifr);
> +       if (ret) {

From the ioctl man page
Usually, on success zero is returned.  A few ioctl() requests use the
       return value as an output parameter and return a nonnegative value on
       success.  On error, -1 is returned, and errno is set appropriately.

So in this case you need to check for (-1==ret)


> +               ODP_DBG("ioctl SIOCGIFMTU error\n");
> +               return -1;
> +       }
> +
> +       return ifr.ifr_mtu;
> +}
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Maxim Uvarov Nov. 15, 2014, 9:25 a.m. UTC | #2
On 11/14/2014 10:50 PM, Mike Holmes wrote:
>> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
>> >+       if (ret) {
>  From the ioctl man page
> Usually, on success zero is returned.  A few ioctl() requests use the
>         return value as an output parameter and return a nonnegative value on
>         success.  On error, -1 is returned, and errno is set appropriately.
>
> So in this case you need to check for (-1==ret)
>

Mike, yes I saw that. But I don't think it's needed for ioctls which I'm 
using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any 
other value is error for that ioctls.

Maxim.
Mike Holmes Nov. 15, 2014, 2:30 p.m. UTC | #3
On 15 November 2014 04:25, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> On 11/14/2014 10:50 PM, Mike Holmes wrote:
>>>
>>> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
>>> >+       if (ret) {
>>
>>  From the ioctl man page
>> Usually, on success zero is returned.  A few ioctl() requests use the
>>         return value as an output parameter and return a nonnegative value
>> on
>>         success.  On error, -1 is returned, and errno is set
>> appropriately.
>>
>> So in this case you need to check for (-1==ret)
>>
>
> Mike, yes I saw that. But I don't think it's needed for ioctls which I'm
> using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any
> other value is error for that ioctls.

Ok, I don't have any more comments

>
> Maxim.
Mike Holmes Nov. 15, 2014, 2:31 p.m. UTC | #4
On 14 November 2014 04:30, Maxim Uvarov <maxim.uvarov@linaro.org> 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 <maxim.uvarov@linaro.org>

Reviewed-by Mike Holmes <mike.holmes@linaro.org>

> ---
>  platform/linux-generic/include/api/odp_packet_io.h | 23 ++++++++
>  .../linux-generic/include/odp_packet_io_internal.h |  4 ++
>  platform/linux-generic/odp_packet_io.c             | 66 ++++++++++++++++++++++
>  3 files changed, 93 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..e5cf320 100644
> --- a/platform/linux-generic/include/api/odp_packet_io.h
> +++ b/platform/linux-generic/include/api/odp_packet_io.h
> @@ -136,6 +136,29 @@ 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.
> + *
> + * @param[in] id   ODP packet IO handle.
> + * @param[in] mtu  The value of MTU that the interface will be configured to
> + *                use.
> + *
> + * @retval  0 on success.
> + * @retval -1 if specified mtu can not be handled.
> + * @retval -1 on any other error or illegal input parameters.
> + */
> +int odp_pktio_set_mtu(odp_pktio_t id, int mtu);
> +
> +/**
> + * Return the currently configured MTU value of a packet IO interface.
> + *
> + * @param[in] id  ODP packet IO handle.
> + *
> + * @retval MTU value >0 on success.
> + * @retval -1 on any error or not existance pktio id.
> + */
> +int odp_pktio_mtu(odp_pktio_t id);
> +
> +/**
>   * @}
>   */
>
> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> index 23633ed..0bc1e21 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() */
>  };
>
>  typedef union {
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index f35193f..f0e361a 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,67 @@ 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, int mtu)
> +{
> +       pktio_entry_t *entry;
> +       int sockfd;
> +       struct ifreq ifr;
> +       int ret;
> +
> +       if (mtu <= 0) {
> +               ODP_DBG("illegal MTU value %d\n", mtu);
> +               return -1;
> +       }
> +
> +       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);
> +       ifr.ifr_mtu = mtu;
> +
> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
> +       if (ret) {
> +               ODP_DBG("ioctl SIOCSIFMTU error\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +int odp_pktio_mtu(odp_pktio_t id)
> +{
> +       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);
> +
> +       ret = ioctl(sockfd, SIOCGIFMTU, &ifr);
> +       if (ret) {
> +               ODP_DBG("ioctl SIOCGIFMTU error\n");
> +               return -1;
> +       }
> +
> +       return ifr.ifr_mtu;
> +}
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Anders Roxell Nov. 19, 2014, 9:32 a.m. UTC | #5
On 2014-11-15 12:25, Maxim Uvarov wrote:
> On 11/14/2014 10:50 PM, Mike Holmes wrote:
> >>+       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
> >>>+       if (ret) {
> > From the ioctl man page
> >Usually, on success zero is returned.  A few ioctl() requests use the
> >        return value as an output parameter and return a nonnegative value on
> >        success.  On error, -1 is returned, and errno is set appropriately.
> >
> >So in this case you need to check for (-1==ret)
> >
> 
> Mike, yes I saw that. But I don't think it's needed for ioctls which I'm
> using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any
> other value is error for that ioctls.

Every person that reads this will wounder why you check for different
for zero instead of different from -1.

Be complient with the ioctl manpage please.

Cheers,
Anders
Maxim Uvarov Nov. 19, 2014, 11:39 a.m. UTC | #6
On 11/19/2014 12:32 PM, Anders Roxell wrote:
> On 2014-11-15 12:25, Maxim Uvarov wrote:
>> On 11/14/2014 10:50 PM, Mike Holmes wrote:
>>>> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
>>>>> +       if (ret) {
>>>  From the ioctl man page
>>> Usually, on success zero is returned.  A few ioctl() requests use the
>>>         return value as an output parameter and return a nonnegative value on
>>>         success.  On error, -1 is returned, and errno is set appropriately.
>>>
>>> So in this case you need to check for (-1==ret)
>>>
>> Mike, yes I saw that. But I don't think it's needed for ioctls which I'm
>> using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any
>> other value is error for that ioctls.
> Every person that reads this will wounder why you check for different
> for zero instead of different from -1.
>
> Be complient with the ioctl manpage please.
>
> Cheers,
> Anders
You can check:
1. if ioctl returned not success (0).
2. if ioctl returned error -1.

I prefer to see success in success case. If not success, then rise error.


However if it's disputable place I can follow upstream projects code for 
that ioctl:

ifconfig code:
             if (ioctl(skfd, SIOCSIFMTU, &ifr) < 0) {
                 fprintf(stderr, "SIOCSIFMTU: %s\n", strerror(errno));
                 goterr = 1;
             }
if (ioctl(skfd, SIOCGIFFLAGS, &ifr) < 0)


iproute code:
         err = ioctl(fd, SIOCSIFFLAGS, &ifr);
         if (err)
                         perror("SIOCSIFFLAGS");

         if (ioctl(s, SIOCSIFMTU, &ifr) < 0) {
                 perror("SIOCSIFMTU");


Check for <0 is also not clear. I prefer to see success there. But I 
just follow upstream projects.
Will that work for you?

Maxim.
Anders Roxell Nov. 19, 2014, 3:18 p.m. UTC | #7
On 2014-11-19 14:39, Maxim Uvarov wrote:
> On 11/19/2014 12:32 PM, Anders Roxell wrote:
> >On 2014-11-15 12:25, Maxim Uvarov wrote:
> >>On 11/14/2014 10:50 PM, Mike Holmes wrote:
> >>>>+       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
> >>>>>+       if (ret) {
> >>> From the ioctl man page
> >>>Usually, on success zero is returned.  A few ioctl() requests use the
> >>>        return value as an output parameter and return a nonnegative value on
> >>>        success.  On error, -1 is returned, and errno is set appropriately.
> >>>
> >>>So in this case you need to check for (-1==ret)
> >>>
> >>Mike, yes I saw that. But I don't think it's needed for ioctls which I'm
> >>using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any
> >>other value is error for that ioctls.
> >Every person that reads this will wounder why you check for different
> >for zero instead of different from -1.
> >
> >Be complient with the ioctl manpage please.
> >
> >Cheers,
> >Anders
> You can check:
> 1. if ioctl returned not success (0).
> 2. if ioctl returned error -1.
> 
> I prefer to see success in success case. If not success, then rise error.
> 
> 
> However if it's disputable place I can follow upstream projects code for
> that ioctl:
> 
> ifconfig code:
>             if (ioctl(skfd, SIOCSIFMTU, &ifr) < 0) {
>                 fprintf(stderr, "SIOCSIFMTU: %s\n", strerror(errno));
>                 goterr = 1;
>             }
> if (ioctl(skfd, SIOCGIFFLAGS, &ifr) < 0)
> 
> 
> iproute code:
>         err = ioctl(fd, SIOCSIFFLAGS, &ifr);
>         if (err)
>                         perror("SIOCSIFFLAGS");
> 
>         if (ioctl(s, SIOCSIFMTU, &ifr) < 0) {
>                 perror("SIOCSIFMTU");
> 
> 
> Check for <0 is also not clear. I prefer to see success there. But I just
> follow upstream projects.
> Will that work for you?

For simplicity and to remove the need for understanding the meaning of
the return value for the specific ioctl call I think a comparison with
-1 the right thing to do, since this for *all* ioctl calls means error.


Cheers,
Anders
Maxim Uvarov Nov. 19, 2014, 3:34 p.m. UTC | #8
On 11/19/2014 06:18 PM, Anders Roxell wrote:
> On 2014-11-19 14:39, Maxim Uvarov wrote:
>> On 11/19/2014 12:32 PM, Anders Roxell wrote:
>>> On 2014-11-15 12:25, Maxim Uvarov wrote:
>>>> On 11/14/2014 10:50 PM, Mike Holmes wrote:
>>>>>> +       ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
>>>>>>> +       if (ret) {
>>>>>  From the ioctl man page
>>>>> Usually, on success zero is returned.  A few ioctl() requests use the
>>>>>         return value as an output parameter and return a nonnegative value on
>>>>>         success.  On error, -1 is returned, and errno is set appropriately.
>>>>>
>>>>> So in this case you need to check for (-1==ret)
>>>>>
>>>> Mike, yes I saw that. But I don't think it's needed for ioctls which I'm
>>>> using here. I.e. for SIOCSIFMTU and other. I want them to return 0. Any
>>>> other value is error for that ioctls.
>>> Every person that reads this will wounder why you check for different
>>> for zero instead of different from -1.
>>>
>>> Be complient with the ioctl manpage please.
>>>
>>> Cheers,
>>> Anders
>> You can check:
>> 1. if ioctl returned not success (0).
>> 2. if ioctl returned error -1.
>>
>> I prefer to see success in success case. If not success, then rise error.
>>
>>
>> However if it's disputable place I can follow upstream projects code for
>> that ioctl:
>>
>> ifconfig code:
>>              if (ioctl(skfd, SIOCSIFMTU, &ifr) < 0) {
>>                  fprintf(stderr, "SIOCSIFMTU: %s\n", strerror(errno));
>>                  goterr = 1;
>>              }
>> if (ioctl(skfd, SIOCGIFFLAGS, &ifr) < 0)
>>
>>
>> iproute code:
>>          err = ioctl(fd, SIOCSIFFLAGS, &ifr);
>>          if (err)
>>                          perror("SIOCSIFFLAGS");
>>
>>          if (ioctl(s, SIOCSIFMTU, &ifr) < 0) {
>>                  perror("SIOCSIFMTU");
>>
>>
>> Check for <0 is also not clear. I prefer to see success there. But I just
>> follow upstream projects.
>> Will that work for you?
> For simplicity and to remove the need for understanding the meaning of
> the return value for the specific ioctl call I think a comparison with
> -1 the right thing to do, since this for *all* ioctl calls means error.
>
>
> Cheers,
> Anders
Why ifconfig has <0 check? Might it be different in other systems BSD, 
Solaris, etc?

Maxim.
Maxim Uvarov Nov. 19, 2014, 3:56 p.m. UTC | #9
Anders, check for -1 is not correct. It does not matter what is in man 
page. Don't read documentation :)
Look the kernel code:

./net/core/dev_ioctl.c:
     case SIOCSIFMTU:    /* Set the MTU of a device */
         return dev_set_mtu(dev, ifr->ifr_mtu);

./net/core/dev.c
/**
  *    dev_set_mtu - Change maximum transfer unit
  *    @dev: device
  *    @new_mtu: new transfer unit
  *
  *    Change the maximum transfer size of the network device.
  */
int dev_set_mtu(struct net_device *dev, int new_mtu)
{
     int err, orig_mtu;

     if (new_mtu == dev->mtu)
         return 0;

     /*    MTU must be positive.     */
     if (new_mtu < 0)
         return -EINVAL;

     if (!netif_device_present(dev))
         return -ENODEV;

     err = call_netdevice_notifiers(NETDEV_PRECHANGEMTU, dev);
     err = notifier_to_errno(err);
     if (err)
         return err;

     orig_mtu = dev->mtu;
     err = __dev_set_mtu(dev, new_mtu);

     if (!err) {
         err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
         err = notifier_to_errno(err);
         if (err) {
             /* setting mtu back and notifying everyone again,
              * so that they have a chance to revert changes.
              */
             __dev_set_mtu(dev, orig_mtu);
             call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
         }
     }
     return err;
}

So I'm going to stay with mine solution. Did I convince you now? ;)

Maxim.
Anders Roxell Nov. 19, 2014, 7:37 p.m. UTC | #10
On 19 November 2014 16:56, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> Anders, check for -1 is not correct. It does not matter what is in man page.
> Don't read documentation :)

I'm sorry for not doing my homework! =)

> Look the kernel code:
>
> ./net/core/dev_ioctl.c:
>     case SIOCSIFMTU:    /* Set the MTU of a device */
>         return dev_set_mtu(dev, ifr->ifr_mtu);
>
> ./net/core/dev.c
> /**
>  *    dev_set_mtu - Change maximum transfer unit
>  *    @dev: device
>  *    @new_mtu: new transfer unit
>  *
>  *    Change the maximum transfer size of the network device.
>  */
> int dev_set_mtu(struct net_device *dev, int new_mtu)
> {
>     int err, orig_mtu;
>
>     if (new_mtu == dev->mtu)
>         return 0;
>
>     /*    MTU must be positive.     */
>     if (new_mtu < 0)
>         return -EINVAL;
>
>     if (!netif_device_present(dev))
>         return -ENODEV;
>
>     err = call_netdevice_notifiers(NETDEV_PRECHANGEMTU, dev);
>     err = notifier_to_errno(err);
>     if (err)
>         return err;
>
>     orig_mtu = dev->mtu;
>     err = __dev_set_mtu(dev, new_mtu);
>
>     if (!err) {
>         err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
>         err = notifier_to_errno(err);
>         if (err) {
>             /* setting mtu back and notifying everyone again,
>              * so that they have a chance to revert changes.
>              */
>             __dev_set_mtu(dev, orig_mtu);
>             call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
>         }
>     }
>     return err;
> }
>
> So I'm going to stay with mine solution. Did I convince you now? ;)

You convinced me that the documentation are wrong!

It is still better to check for < 0 and not only for !0 because there
are non-negative values
for success.

Because it's confusing to only check for zero in this case when ioctl
can return different values on success.

So if you don't agree than changing to < 0 it would be worth adding a
comment about it.

Cheers,
Anders
diff mbox

Patch

diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h
index 360636d..e5cf320 100644
--- a/platform/linux-generic/include/api/odp_packet_io.h
+++ b/platform/linux-generic/include/api/odp_packet_io.h
@@ -136,6 +136,29 @@  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.
+ *
+ * @param[in] id   ODP packet IO handle.
+ * @param[in] mtu  The value of MTU that the interface will be configured to
+ *		   use.
+ *
+ * @retval  0 on success.
+ * @retval -1 if specified mtu can not be handled.
+ * @retval -1 on any other error or illegal input parameters.
+ */
+int odp_pktio_set_mtu(odp_pktio_t id, int mtu);
+
+/**
+ * Return the currently configured MTU value of a packet IO interface.
+ *
+ * @param[in] id  ODP packet IO handle.
+ *
+ * @retval MTU value >0 on success.
+ * @retval -1 on any error or not existance pktio id.
+ */
+int odp_pktio_mtu(odp_pktio_t id);
+
+/**
  * @}
  */
 
diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
index 23633ed..0bc1e21 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() */
 };
 
 typedef union {
diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
index f35193f..f0e361a 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,67 @@  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, int mtu)
+{
+	pktio_entry_t *entry;
+	int sockfd;
+	struct ifreq ifr;
+	int ret;
+
+	if (mtu <= 0) {
+		ODP_DBG("illegal MTU value %d\n", mtu);
+		return -1;
+	}
+
+	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);
+	ifr.ifr_mtu = mtu;
+
+	ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
+	if (ret) {
+		ODP_DBG("ioctl SIOCSIFMTU error\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+int odp_pktio_mtu(odp_pktio_t id)
+{
+	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);
+
+	ret = ioctl(sockfd, SIOCGIFMTU, &ifr);
+	if (ret) {
+		ODP_DBG("ioctl SIOCGIFMTU error\n");
+		return -1;
+	}
+
+	return ifr.ifr_mtu;
+}