[ovs-dev] [PATCH 3/4] ovs-brcompatd: Delete what Bridge references when deleting a Bridge.
Ben Pfaff
blp at nicira.com
Thu Feb 25 23:54:49 UTC 2010
On Wed, Feb 24, 2010 at 11:10:12PM -0800, Justin Pettit wrote:
> On Feb 23, 2010, at 2:36 PM, Ben Pfaff wrote:
> > [...]
>
> It seems like this rewrite of del_port() will destroy a port even if
> it's just an interface. So for example, let's say that "eth0" and
> "eth1" are interfaces as part of the port "bond0". If someone
> attempts to delete "eth0", then it appears that "bond0" will be
> destroyed along with "eth0" and "eth1". Is this correct, and if so,
> is this what we want? I guess I would have expected it just to remove
> "eth0" from "bond0" and then destroy the "eth0" record.
You are correct. I thought about that case but discounted it because
only physical interfaces are normally bonded and physical interfaces
don't normally disappear.
I guess that the right thing to do is to remove the interface, then, if
the port no longer has any interfaces, remove the port.
I've reworked the patch to do that. I'm appending the updated version.
> > @@ -496,10 +521,22 @@ del_bridge(const struct ovsrec_open_vswitch *ovs, const char *br_name)
> > return ENXIO;
> > }
> >
> > - del_port(br, br_name);
> > -
> > - ovsrec_bridge_delete(br);
> > + /* Delete everything that the bridge points to, then delete the bridge
> > + * itself. */
> > + while (br->n_ports > 0) {
> > + del_bridge_port(br, br->ports[0]);
> > + }
> > + for (i = 0; i < br->n_mirrors; i++) {
> > + ovsrec_mirror_delete(br->mirrors[i]);
>
> Is there a place where "br->n_mirrors" is getting decremented by the
> delete? This goes into to some auto-generated IDL code that I haven't
> fully gotten my head around but I don't see where it does.
It does not decrement br->n_mirrors or otherwise remove the mirror from
the bridge's list of mirrors, and the IDL code does not do that either.
But there is no need to do so, because the Bridge record is also getting
deleted and so its contents do not matter.
> > + }
> > + if (br->netflow) {
> > + ovsrec_netflow_delete(br->netflow);
> > + }
> > + if (br->sflow) {
> > + ovsrec_sflow_delete(br->sflow);
> > + }
>
> Are all of these items guaranteed to not be shared by other bridges?
> Is there something preventing multiple bridges from sharing a newflow
> or sflow record? If so, then it seems like this could leave a
> dangling reference for those other bridges.
There are a few different philosophies we could follow here. One is
that sharing is allowed. I'm a little worried about that, because it
takes extra care by database users to make sure to delete records only
when there are no references left. That could be integrated into the
database itself, but it is not implemented now.
Another is that only a single "owner" points to, say, a NetFlow or sFlow
record. This seems simpler, because no checking is needed at deletion
time. If record A points to B, then deleting A should delete B also. I
am in favor of that, for now at least, because of its simplicity.
This needs to be documented. I am working on documentation for the
database schema, but it is not ready yet.
> Also, it seems like a controller record would also fall in this
> category of needing to be deleted (with the same caveat that it could
> be shared).
Yes. I missed the controller record somehow. Thanks for pointing that
out.
Here is a revised patch for another look. I am in the process of
testing it.
--8<--------------------------cut here-------------------------->8--
>From e753f1ba092adc5ee28c1d5860ce7072a5d4886d Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 25 Feb 2010 15:52:12 -0800
Subject: [PATCH] ovs-brcompatd: Delete what Bridge references when deleting a Bridge.
A Bridge record can reference a number of other records: Port, Mirror,
NetFlow, sFlow, and Controller records. When the Bridge is deleted, we
should also delete those records that it references. This commit does
that.
Bug #2425.
---
vswitchd/ovs-brcompatd.c | 137 ++++++++++++++++++++++++++++++++++++---------
1 files changed, 109 insertions(+), 28 deletions(-)
diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
index bf571d7..0d378d7 100644
--- a/vswitchd/ovs-brcompatd.c
+++ b/vswitchd/ovs-brcompatd.c
@@ -447,41 +447,98 @@ add_port(const struct ovsrec_open_vswitch *ovs,
free(ports);
}
+/* Deletes 'port' from 'br'.
+ *
+ * After calling this function, 'port' must not be referenced again. */
static void
-del_port(const struct ovsrec_bridge *br, const char *port_name)
+del_port(const struct ovsrec_bridge *br, const struct ovsrec_port *port)
{
- size_t i, j;
- struct ovsrec_port *port_rec = NULL;
+ struct ovsrec_port **ports;
+ size_t i, n;
+
+ /* Remove 'port' from the bridge's list of ports. */
+ ports = xmalloc(sizeof *br->ports * br->n_ports);
+ for (i = n = 0; i < br->n_ports; i++) {
+ if (br->ports[i] != port) {
+ ports[n++] = br->ports[i];
+ }
+ }
+ ovsrec_bridge_set_ports(br, ports, n);
+ free(ports);
+
+ /* Delete all of the port's interfaces. */
+ for (i = 0; i < port->n_interfaces; i++) {
+ ovsrec_interface_delete(port->interfaces[i]);
+ }
+
+ /* Delete the port itself. */
+ ovsrec_port_delete(port);
+}
+
+/* Delete 'iface' from 'port' (which must be within 'br'). If 'iface' was
+ * 'port''s only interface, delete 'port' from 'br' also.
+ *
+ * After calling this function, 'iface' must not be referenced again. */
+static void
+del_interface(const struct ovsrec_bridge *br,
+ const struct ovsrec_port *port,
+ const struct ovsrec_interface *iface)
+{
+ if (port->n_interfaces == 1) {
+ del_port(br, port);
+ } else {
+ struct ovsrec_interface **ifaces;
+ size_t i, n;
+
+ ifaces = xmalloc(sizeof *port->interfaces * port->n_interfaces);
+ for (i = n = 0; i < port->n_interfaces; i++) {
+ if (port->interfaces[i] != iface) {
+ ifaces[n++] = port->interfaces[i];
+ }
+ }
+ ovsrec_port_set_interfaces(port, ifaces, n);
+ free(ifaces);
+ ovsrec_interface_delete(iface);
+ }
+}
+
+/* Find and return a port within 'br' named 'port_name'. */
+static const struct ovsrec_port *
+find_port(const struct ovsrec_bridge *br, const char *port_name)
+{
+ size_t i;
for (i = 0; i < br->n_ports; i++) {
struct ovsrec_port *port = br->ports[i];
if (!strcmp(port_name, port->name)) {
- port_rec = port;
- }
- for (j = 0; j < port->n_interfaces; j++) {
- struct ovsrec_interface *iface = port->interfaces[j];
- if (!strcmp(port_name, iface->name)) {
- ovsrec_interface_delete(iface);
- }
+ return port;
}
}
+ return NULL;
+}
- /* xxx Probably can move this into the "for" loop. */
- if (port_rec) {
- struct ovsrec_port **ports;
- size_t n;
+/* Find and return an interface within 'br' named 'iface_name'. */
+static const struct ovsrec_interface *
+find_interface(const struct ovsrec_bridge *br, const char *iface_name,
+ struct ovsrec_port **portp)
+{
+ size_t i;
+
+ for (i = 0; i < br->n_ports; i++) {
+ struct ovsrec_port *port = br->ports[i];
+ size_t j;
- ports = xmalloc(sizeof *br->ports * br->n_ports);
- for (i = n = 0; i < br->n_ports; i++) {
- if (br->ports[i] != port_rec) {
- ports[n++] = br->ports[i];
+ for (j = 0; j < port->n_interfaces; j++) {
+ struct ovsrec_interface *iface = port->interfaces[j];
+ if (!strcmp(iface->name, iface_name)) {
+ *portp = port;
+ return iface;
}
}
- ovsrec_bridge_set_ports(br, ports, n);
- free(ports);
-
- ovsrec_port_delete(port_rec);
}
+
+ *portp = NULL;
+ return NULL;
}
static int
@@ -496,10 +553,25 @@ del_bridge(const struct ovsrec_open_vswitch *ovs, const char *br_name)
return ENXIO;
}
- del_port(br, br_name);
-
- ovsrec_bridge_delete(br);
+ /* Delete everything that the bridge points to, then delete the bridge
+ * itself. */
+ while (br->n_ports > 0) {
+ del_port(br, br->ports[0]);
+ }
+ for (i = 0; i < br->n_mirrors; i++) {
+ ovsrec_mirror_delete(br->mirrors[i]);
+ }
+ if (br->netflow) {
+ ovsrec_netflow_delete(br->netflow);
+ }
+ if (br->sflow) {
+ ovsrec_sflow_delete(br->sflow);
+ }
+ if (br->controller) {
+ ovsrec_controller_delete(br->controller);
+ }
+ /* Remove 'br' from the vswitch's list of bridges. */
bridges = xmalloc(sizeof *ovs->bridges * ovs->n_bridges);
for (i = n = 0; i < ovs->n_bridges; i++) {
if (ovs->bridges[i] != br) {
@@ -634,7 +706,10 @@ handle_port_cmd(const struct ovsrec_open_vswitch *ovs,
if (add) {
add_port(ovs, br, port_name);
} else {
- del_port(br, port_name);
+ const struct ovsrec_port *port = find_port(br, port_name);
+ if (port) {
+ del_port(br, port);
+ }
}
VLOG_INFO("%s %s %s: success", cmd_name, br_name, port_name);
}
@@ -1077,11 +1152,14 @@ rtnl_recv_update(const struct ovsrec_open_vswitch *ovs)
if (!netdev_exists(port_name)) {
/* Network device is really gone. */
- struct ovsrec_bridge *br = find_bridge(ovs, br_name);
+ const struct ovsrec_interface *iface;
+ struct ovsrec_port *port;
+ struct ovsrec_bridge *br;
VLOG_INFO("network device %s destroyed, "
"removing from bridge %s", port_name, br_name);
+ br = find_bridge(ovs, br_name);
if (!br) {
VLOG_WARN("no bridge named %s from which to remove %s",
br_name, port_name);
@@ -1089,7 +1167,10 @@ rtnl_recv_update(const struct ovsrec_open_vswitch *ovs)
return;
}
- del_port(br, port_name);
+ iface = find_interface(br, port_name, &port);
+ if (iface) {
+ del_interface(br, port, iface);
+ }
} else {
/* A network device by that name exists even though the kernel
* told us it had disappeared. Probably, what happened was
--
1.6.6.1
More information about the dev
mailing list