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

Ben Pfaff blp at nicira.com
Thu Jun 6 23:58:39 UTC 2013


Thanks, applied to master.

On Thu, Jun 06, 2013 at 01:33:17PM -0700, Andy Zhou wrote:
> 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
> >



More information about the dev mailing list