[ovs-dev] [PATCH 3/4] ovs-brcompatd: Delete what Bridge references when deleting a Bridge.

Justin Pettit jpettit at nicira.com
Thu Feb 25 07:10:12 UTC 2010


On Feb 23, 2010, at 2:36 PM, Ben Pfaff wrote:

> +static const struct ovsrec_port *
> +find_port(const struct ovsrec_bridge *br, const char *port_name)
> +{
> +    size_t i;
> +
> +    /* Find a port named 'port_name'. */
>     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;
>         }
>     }
> 
> -    /* xxx Probably can move this into the "for" loop. */
> -    if (port_rec) {
> -        struct ovsrec_port **ports;
> -        size_t n;
> +    /* Find a port with an interface named 'port_name'. */
> +    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++) {
> +            if (!strcmp(port->interfaces[j]->name, port_name)) {
> +                return port;
>             }
>         }
> -        ovsrec_bridge_set_ports(br, ports, n);
> -        free(ports);
> +    }
> 
> -        ovsrec_port_delete(port_rec);
> +    return NULL;
> +}
> +
> +static void
> +del_port(const struct ovsrec_bridge *br, const char *port_name)
> +{
> +    const struct ovsrec_port *port = find_port(br, port_name);
> +    if (port) {
> +        del_bridge_port(br, port);
>     }
> }

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.

> @@ -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.

> +    }
> +    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.  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).

--Justin






More information about the dev mailing list