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 <diproiet...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
