| Message ID | 1456690403-15050-2-git-send-email-diproiettod@vmware.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Daniele, One trivial comment below, but other than that, LGTM. Cheers, Mark > >This fixes multiple error path mistakes in do_add_port, none of which >has been a problem in practice so far. This change will make it easier >for a following commit to return in case of error. > >Also, this removes an unneeded special case for tunnel ports. > >Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >--- > lib/dpif-netdev.c | 50 +++++++++++++++++++++++++++----------------------- > 1 file changed, 27 insertions(+), 23 deletions(-) > >diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >index a7e224a..914579d 100644 >--- a/lib/dpif-netdev.c >+++ b/lib/dpif-netdev.c >@@ -1106,27 +1106,28 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char >*type, > struct netdev *netdev; > enum netdev_flags flags; > const char *open_type; >- int error; >- int i; >+ int error = 0; >+ int i, n_open_rxqs; > > /* Reject devices already in 'dp'. */ > if (!get_port_by_name(dp, devname, &port)) { >- return EEXIST; >+ error = EEXIST; >+ goto out; > } > > /* Open and validate network device. */ > open_type = dpif_netdev_port_open_type(dp->class, type); > error = netdev_open(devname, open_type, &netdev); > if (error) { >- return error; >+ goto out; > } > /* XXX reject non-Ethernet devices */ > > netdev_get_flags(netdev, &flags); > if (flags & NETDEV_LOOPBACK) { > VLOG_ERR("%s: cannot add a loopback device", devname); >- netdev_close(netdev); >- return EINVAL; >+ error = EINVAL; >+ goto out_close; > } > > if (netdev_is_pmd(netdev)) { >@@ -1134,7 +1135,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char >*type, > > if (n_cores == OVS_CORE_UNSPEC) { > VLOG_ERR("%s, cannot get cpu core info", devname); >- return ENOENT; >+ error = ENOENT; >+ goto out_close; > } > /* There can only be ovs_numa_get_n_cores() pmd threads, > * so creates a txq for each, and one extra for the non >@@ -1143,7 +1145,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char >*type, > netdev_requested_n_rxq(netdev)); > if (error && (error != EOPNOTSUPP)) { > VLOG_ERR("%s, cannot set multiq", devname); >- return errno; >+ goto out_close; > } > } > port = xzalloc(sizeof *port); >@@ -1152,30 +1154,20 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char >*type, > port->rxq = xmalloc(sizeof *port->rxq * netdev_n_rxq(netdev)); > port->type = xstrdup(type); > port->latest_requested_n_rxq = netdev_requested_n_rxq(netdev); >+ n_open_rxqs = 0; You could just initialize n_open_rxqs in its declaration, but no biggie. > for (i = 0; i < netdev_n_rxq(netdev); i++) { > error = netdev_rxq_open(netdev, &port->rxq[i], i); >- if (error >- && !(error == EOPNOTSUPP && dpif_netdev_class_is_dummy(dp->class))) { >+ if (error) { > VLOG_ERR("%s: cannot receive packets on this network device (%s)", > devname, ovs_strerror(errno)); >- netdev_close(netdev); >- free(port->type); >- free(port->rxq); >- free(port); >- return error; >+ goto out_rxq_close; > } >+ n_open_rxqs++; > } > > error = netdev_turn_flags_on(netdev, NETDEV_PROMISC, &sf); > if (error) { >- for (i = 0; i < netdev_n_rxq(netdev); i++) { >- netdev_rxq_close(port->rxq[i]); >- } >- netdev_close(netdev); >- free(port->type); >- free(port->rxq); >- free(port); >- return error; >+ goto out_rxq_close; > } > port->sf = sf; > >@@ -1188,6 +1180,18 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char >*type, > seq_change(dp->port_seq); > > return 0; >+ >+out_rxq_close: >+ for (i = 0; i < n_open_rxqs; i++) { >+ netdev_rxq_close(port->rxq[i]); >+ } >+ free(port->type); >+ free(port->rxq); >+ free(port); >+out_close: >+ netdev_close(netdev); >+out: >+ return error; > } > > static int >-- >2.1.4 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
On 01/03/2016 09:10, "Kavanagh, Mark B" <mark.b.kavanagh@intel.com> wrote: >Hi Daniele, > >One trivial comment below, but other than that, LGTM. > >Cheers, >Mark > >> >>This fixes multiple error path mistakes in do_add_port, none of which >>has been a problem in practice so far. This change will make it easier >>for a following commit to return in case of error. >> >>Also, this removes an unneeded special case for tunnel ports. >> >>Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >>--- >> lib/dpif-netdev.c | 50 >>+++++++++++++++++++++++++++----------------------- >> 1 file changed, 27 insertions(+), 23 deletions(-) >> >>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>index a7e224a..914579d 100644 >>--- a/lib/dpif-netdev.c >>+++ b/lib/dpif-netdev.c >>@@ -1106,27 +1106,28 @@ do_add_port(struct dp_netdev *dp, const char >>*devname, const char >>*type, >> struct netdev *netdev; >> enum netdev_flags flags; >> const char *open_type; >>- int error; >>- int i; >>+ int error = 0; >>+ int i, n_open_rxqs; >> >> /* Reject devices already in 'dp'. */ >> if (!get_port_by_name(dp, devname, &port)) { >>- return EEXIST; >>+ error = EEXIST; >>+ goto out; >> } >> >> /* Open and validate network device. */ >> open_type = dpif_netdev_port_open_type(dp->class, type); >> error = netdev_open(devname, open_type, &netdev); >> if (error) { >>- return error; >>+ goto out; >> } >> /* XXX reject non-Ethernet devices */ >> >> netdev_get_flags(netdev, &flags); >> if (flags & NETDEV_LOOPBACK) { >> VLOG_ERR("%s: cannot add a loopback device", devname); >>- netdev_close(netdev); >>- return EINVAL; >>+ error = EINVAL; >>+ goto out_close; >> } >> >> if (netdev_is_pmd(netdev)) { >>@@ -1134,7 +1135,8 @@ do_add_port(struct dp_netdev *dp, const char >>*devname, const char >>*type, >> >> if (n_cores == OVS_CORE_UNSPEC) { >> VLOG_ERR("%s, cannot get cpu core info", devname); >>- return ENOENT; >>+ error = ENOENT; >>+ goto out_close; >> } >> /* There can only be ovs_numa_get_n_cores() pmd threads, >> * so creates a txq for each, and one extra for the non >>@@ -1143,7 +1145,7 @@ do_add_port(struct dp_netdev *dp, const char >>*devname, const char >>*type, >> netdev_requested_n_rxq(netdev)); >> if (error && (error != EOPNOTSUPP)) { >> VLOG_ERR("%s, cannot set multiq", devname); >>- return errno; >>+ goto out_close; >> } >> } >> port = xzalloc(sizeof *port); >>@@ -1152,30 +1154,20 @@ do_add_port(struct dp_netdev *dp, const char >>*devname, const char >>*type, >> port->rxq = xmalloc(sizeof *port->rxq * netdev_n_rxq(netdev)); >> port->type = xstrdup(type); >> port->latest_requested_n_rxq = netdev_requested_n_rxq(netdev); >>+ n_open_rxqs = 0; > >You could just initialize n_open_rxqs in its declaration, but no biggie. Good point, done
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index a7e224a..914579d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1106,27 +1106,28 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, struct netdev *netdev; enum netdev_flags flags; const char *open_type; - int error; - int i; + int error = 0; + int i, n_open_rxqs; /* Reject devices already in 'dp'. */ if (!get_port_by_name(dp, devname, &port)) { - return EEXIST; + error = EEXIST; + goto out; } /* Open and validate network device. */ open_type = dpif_netdev_port_open_type(dp->class, type); error = netdev_open(devname, open_type, &netdev); if (error) { - return error; + goto out; } /* XXX reject non-Ethernet devices */ netdev_get_flags(netdev, &flags); if (flags & NETDEV_LOOPBACK) { VLOG_ERR("%s: cannot add a loopback device", devname); - netdev_close(netdev); - return EINVAL; + error = EINVAL; + goto out_close; } if (netdev_is_pmd(netdev)) { @@ -1134,7 +1135,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, if (n_cores == OVS_CORE_UNSPEC) { VLOG_ERR("%s, cannot get cpu core info", devname); - return ENOENT; + error = ENOENT; + goto out_close; } /* There can only be ovs_numa_get_n_cores() pmd threads, * so creates a txq for each, and one extra for the non @@ -1143,7 +1145,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, netdev_requested_n_rxq(netdev)); if (error && (error != EOPNOTSUPP)) { VLOG_ERR("%s, cannot set multiq", devname); - return errno; + goto out_close; } } port = xzalloc(sizeof *port); @@ -1152,30 +1154,20 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, port->rxq = xmalloc(sizeof *port->rxq * netdev_n_rxq(netdev)); port->type = xstrdup(type); port->latest_requested_n_rxq = netdev_requested_n_rxq(netdev); + n_open_rxqs = 0; for (i = 0; i < netdev_n_rxq(netdev); i++) { error = netdev_rxq_open(netdev, &port->rxq[i], i); - if (error - && !(error == EOPNOTSUPP && dpif_netdev_class_is_dummy(dp->class))) { + if (error) { VLOG_ERR("%s: cannot receive packets on this network device (%s)", devname, ovs_strerror(errno)); - netdev_close(netdev); - free(port->type); - free(port->rxq); - free(port); - return error; + goto out_rxq_close; } + n_open_rxqs++; } error = netdev_turn_flags_on(netdev, NETDEV_PROMISC, &sf); if (error) { - for (i = 0; i < netdev_n_rxq(netdev); i++) { - netdev_rxq_close(port->rxq[i]); - } - netdev_close(netdev); - free(port->type); - free(port->rxq); - free(port); - return error; + goto out_rxq_close; } port->sf = sf; @@ -1188,6 +1180,18 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, seq_change(dp->port_seq); return 0; + +out_rxq_close: + for (i = 0; i < n_open_rxqs; i++) { + netdev_rxq_close(port->rxq[i]); + } + free(port->type); + free(port->rxq); + free(port); +out_close: + netdev_close(netdev); +out: + return error; } static int
This fixes multiple error path mistakes in do_add_port, none of which has been a problem in practice so far. This change will make it easier for a following commit to return in case of error. Also, this removes an unneeded special case for tunnel ports. Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- lib/dpif-netdev.c | 50 +++++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 23 deletions(-)