[ovs-dev] [threads 17/17] netdev-vport: Don't return static data in netdev_vport_get_dpif_port().

Andy Zhou azhou at nicira.com
Thu Jun 6 20:33:17 UTC 2013


Looks good.  Thanks.


On Wed, Jun 5, 2013 at 1:05 PM, Ben Pfaff <blp at nicira.com> wrote:

> Returning a static data buffer makes code more brittle and definitely
> not thread-safe, so this commit switches to using a caller-provided
> buffer instead.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/dpif-linux.c       |    4 +++-
>  lib/dpif-netdev.c      |    9 ++++++---
>  lib/netdev-vport.c     |   18 ++++++++++++++----
>  lib/netdev-vport.h     |    7 ++++++-
>  ofproto/ofproto-dpif.c |   17 +++++++++++++----
>  5 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index ac20ae7..1383b58 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -484,7 +484,9 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev
> *netdev,
>  {
>      struct dpif_linux *dpif = dpif_linux_cast(dpif_);
>      const struct netdev_tunnel_config *tnl_cfg;
> -    const char *name = netdev_vport_get_dpif_port(netdev);
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +    const char *name = netdev_vport_get_dpif_port(netdev,
> +                                                  namebuf, sizeof
> namebuf);
>      const char *type = netdev_get_type(netdev);
>      struct dpif_linux_vport request, reply;
>      struct nl_sock *sock = NULL;
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 9ea8d24..36992a6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -436,8 +436,11 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev
> *netdev,
>                       uint32_t *port_nop)
>  {
>      struct dp_netdev *dp = get_dp_netdev(dpif);
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +    const char *dpif_port;
>      int port_no;
>
> +    dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof
> namebuf);
>      if (*port_nop != UINT32_MAX) {
>          if (*port_nop >= MAX_PORTS) {
>              return EFBIG;
> @@ -446,12 +449,11 @@ dpif_netdev_port_add(struct dpif *dpif, struct
> netdev *netdev,
>          }
>          port_no = *port_nop;
>      } else {
> -        port_no = choose_port(dp, netdev_vport_get_dpif_port(netdev));
> +        port_no = choose_port(dp, dpif_port);
>      }
>      if (port_no >= 0) {
>          *port_nop = port_no;
> -        return do_add_port(dp, netdev_vport_get_dpif_port(netdev),
> -                           netdev_get_type(netdev), port_no);
> +        return do_add_port(dp, dpif_port, netdev_get_type(netdev),
> port_no);
>      }
>      return EFBIG;
>  }
> @@ -1074,6 +1076,7 @@ dpif_netdev_run(struct dpif *dpif)
>              dp_netdev_port_input(dp, port, &packet);
>          } else if (error != EAGAIN && error != EOPNOTSUPP) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
>              VLOG_ERR_RL(&rl, "error receiving data from %s: %s",
>                          netdev_get_name(port->netdev), strerror(error));
>          }
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 4bb41bd..bdb0c4d 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -121,12 +121,12 @@ netdev_vport_class_get_dpif_port(const struct
> netdev_class *class)
>  }
>
>  const char *
> -netdev_vport_get_dpif_port(const struct netdev *netdev)
> +netdev_vport_get_dpif_port(const struct netdev *netdev,
> +                           char namebuf[], size_t bufsize)
>  {
>      if (netdev_vport_needs_dst_port(netdev)) {
>          const struct netdev_vport *vport = netdev_vport_cast(netdev);
>          const char *type = netdev_get_type(netdev);
> -        static char dpif_port_combined[IFNAMSIZ];
>
>          /*
>           * Note: IFNAMSIZ is 16 bytes long. The maximum length of a VXLAN
> @@ -134,10 +134,11 @@ netdev_vport_get_dpif_port(const struct netdev
> *netdev)
>           * assert here on the size of strlen(type) in case that changes
>           * in the future.
>           */
> +        BUILD_ASSERT(NETDEV_VPORT_NAME_BUFSIZE >= IFNAMSIZ);
>          ovs_assert(strlen(type) + 10 < IFNAMSIZ);
> -        snprintf(dpif_port_combined, IFNAMSIZ, "%s_sys_%d", type,
> +        snprintf(namebuf, bufsize, "%s_sys_%d", type,
>                   ntohs(vport->tnl_cfg.dst_port));
> -        return dpif_port_combined;
> +        return namebuf;
>      } else {
>          const struct netdev_class *class = netdev_get_class(netdev);
>          const char *dpif_port = netdev_vport_class_get_dpif_port(class);
> @@ -145,6 +146,15 @@ netdev_vport_get_dpif_port(const struct netdev
> *netdev)
>      }
>  }
>
> +char *
> +netdev_vport_get_dpif_port_strdup(const struct netdev *netdev)
> +{
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +
> +    return xstrdup(netdev_vport_get_dpif_port(netdev, namebuf,
> +                                              sizeof namebuf));
> +}
> +
>  static int
>  netdev_vport_create(const struct netdev_class *netdev_class, const char
> *name,
>                      struct netdev **netdevp)
> diff --git a/lib/netdev-vport.h b/lib/netdev-vport.h
> index 94fe937..5394966 100644
> --- a/lib/netdev-vport.h
> +++ b/lib/netdev-vport.h
> @@ -18,6 +18,7 @@
>  #define NETDEV_VPORT_H 1
>
>  #include <stdbool.h>
> +#include <stddef.h>
>
>  struct dpif_linux_vport;
>  struct dpif_flow_stats;
> @@ -37,7 +38,11 @@ void netdev_vport_inc_rx(const struct netdev *,
>  void netdev_vport_inc_tx(const struct netdev *,
>                           const struct dpif_flow_stats *);
>
> -const char *netdev_vport_get_dpif_port(const struct netdev *);
>  const char *netdev_vport_class_get_dpif_port(const struct netdev_class *);
>
> +enum { NETDEV_VPORT_NAME_BUFSIZE = 16 };
> +const char *netdev_vport_get_dpif_port(const struct netdev *,
> +                                       char namebuf[], size_t bufsize);
> +char *netdev_vport_get_dpif_port_strdup(const struct netdev *);
> +
>  #endif /* netdev-vport.h */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 096f0f2..cfcd4c8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -956,13 +956,15 @@ type_run(const char *type)
>              }
>
>              HMAP_FOR_EACH (iter, up.hmap_node, &ofproto->up.ports) {
> +                char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
>                  const char *dp_port;
>
>                  if (!iter->tnl_port) {
>                      continue;
>                  }
>
> -                dp_port = netdev_vport_get_dpif_port(iter->up.netdev);
> +                dp_port = netdev_vport_get_dpif_port(iter->up.netdev,
> +                                                     namebuf, sizeof
> namebuf);
>                  node = simap_find(&tmp_backers, dp_port);
>                  if (node) {
>                      simap_put(&backer->tnl_backers, dp_port, node->data);
> @@ -1806,6 +1808,7 @@ port_construct(struct ofport *port_)
>      struct ofport_dpif *port = ofport_dpif_cast(port_);
>      struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
>      const struct netdev *netdev = port->up.netdev;
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
>      struct dpif_port dpif_port;
>      int error;
>
> @@ -1834,7 +1837,8 @@ port_construct(struct ofport *port_)
>      }
>
>      error = dpif_port_query_by_name(ofproto->backer->dpif,
> -                                    netdev_vport_get_dpif_port(netdev),
> +                                    netdev_vport_get_dpif_port(netdev,
> namebuf,
> +                                                               sizeof
> namebuf),
>                                      &dpif_port);
>      if (error) {
>          return error;
> @@ -1871,9 +1875,12 @@ port_destruct(struct ofport *port_)
>  {
>      struct ofport_dpif *port = ofport_dpif_cast(port_);
>      struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
> -    const char *dp_port_name =
> netdev_vport_get_dpif_port(port->up.netdev);
>      const char *devname = netdev_get_name(port->up.netdev);
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +    const char *dp_port_name;
>
> +    dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf,
> +                                              sizeof namebuf);
>      if (dpif_port_exists(ofproto->backer->dpif, dp_port_name)) {
>          /* The underlying device is still there, so delete it.  This
>           * happens when the ofproto is being destroyed, since the caller
> @@ -3317,14 +3324,16 @@ static int
>  port_add(struct ofproto *ofproto_, struct netdev *netdev)
>  {
>      struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> -    const char *dp_port_name = netdev_vport_get_dpif_port(netdev);
>      const char *devname = netdev_get_name(netdev);
> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +    const char *dp_port_name;
>
>      if (netdev_vport_is_patch(netdev)) {
>          sset_add(&ofproto->ghost_ports, netdev_get_name(netdev));
>          return 0;
>      }
>
> +    dp_port_name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof
> namebuf);
>      if (!dpif_port_exists(ofproto->backer->dpif, dp_port_name)) {
>          uint32_t port_no = UINT32_MAX;
>          int error;
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130606/2a5ae5ee/attachment-0003.html>


More information about the dev mailing list