From b3da4c50fced6f9c8326c86dae3126bd66a10316 Mon Sep 17 00:00:00 2001 From: Tobias Waldekranz Date: Mon, 16 Dec 2024 16:59:15 +0100 Subject: [PATCH] confd: bridge: Refactor port setup Clean up bridge-port config generation to align with the bridge upper code. - Remove lots of unnecessary guards that we model guaratees are always met (existance of containers and leafs). This reduces indentation depth and increases readability, IMHO. - Split the generation into smaller pieces, each with a clear purpose. - Try to include all edge cases around the tricky subject of PVID migrations, with the actions running in either exit or init, as appropriate. --- src/confd/src/ietf-interfaces.c | 2 +- src/confd/src/ietf-interfaces.h | 4 +- src/confd/src/infix-if-bridge-port.c | 305 +++++++++++++++------------ 3 files changed, 176 insertions(+), 135 deletions(-) diff --git a/src/confd/src/ietf-interfaces.c b/src/confd/src/ietf-interfaces.c index aabbdfc4..7db4cbb1 100644 --- a/src/confd/src/ietf-interfaces.c +++ b/src/confd/src/ietf-interfaces.c @@ -601,7 +601,7 @@ static sr_error_t netdag_gen_iface(sr_session_ctx_t *session, struct dagger *net fputc('\n', ip); - err = bridge_gen_ports(net, dif, cif, ip); + err = ixif_br_port_gen(dif, cif, ip); if (err) goto err_close_ip; diff --git a/src/confd/src/ietf-interfaces.h b/src/confd/src/ietf-interfaces.h index 925145fe..cda3eac1 100644 --- a/src/confd/src/ietf-interfaces.h +++ b/src/confd/src/ietf-interfaces.h @@ -59,12 +59,10 @@ int netdag_gen_ip_addrs(struct dagger *net, FILE *ip, const char *proto, /* infix-if-bridge.c */ int ixif_br_gen(struct lyd_node *dif, struct lyd_node *cif, FILE *ip, int add); - /* infix-if-bridge-mcd.c */ int ixif_br_mcd_gen(struct lyd_node *cifs); - /* infix-if-bridge-port.c */ -int bridge_gen_ports(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip); +int ixif_br_port_gen(struct lyd_node *dif, struct lyd_node *cif, FILE *ip); /* infix-if-veth.c */ int ifchange_cand_infer_veth(sr_session_ctx_t *session, const char *path); diff --git a/src/confd/src/infix-if-bridge-port.c b/src/confd/src/infix-if-bridge-port.c index 6637f4be..d74ed96e 100644 --- a/src/confd/src/infix-if-bridge-port.c +++ b/src/confd/src/infix-if-bridge-port.c @@ -12,78 +12,154 @@ #include "ietf-interfaces.h" -static void brport_pvid_adjust(FILE *br, struct lyd_node *vlan, int vid, const char *brport, - struct lydx_diff *pvidiff, int tagged) +static const char *ixif_br_port_get_egress_mode(const char *iface, int vid, + const char *brname) { - const char *type = tagged ? "tagged" : "untagged"; - struct lyd_node *port; + static const char *modes[] = { "tagged", "untagged", NULL }; + const char **mode; - LYX_LIST_FOR_EACH(lyd_child(vlan), port, type) { - if (strcmp(brport, lyd_get_value(port))) - continue; - - if (pvidiff->old && atoi(pvidiff->old) == vid) - fprintf(br, "vlan add vid %d dev %s %s\n", vid, brport, type); - if (pvidiff->new && atoi(pvidiff->new) == vid) - fprintf(br, "vlan add vid %d dev %s pvid %s\n", vid, brport, type); + for (mode = modes; *mode; mode++) { + if (srx_get_str(confd.netdag.session, + "/interfaces/interface[name='%s']" + "/bridge/vlans/vlan[vid=%d]/%s[.='%s']", + brname, vid, *mode, iface)) + return *mode; } + + return NULL; } -/* - * Called when only pvid is changed for a bridge-port. Then we use the - * cif data to iterate over all known VLANS for the given port. - */ -static int bridge_port_vlans(struct dagger *net, struct lyd_node *cif, const char *brname, - const char *brport, struct lydx_diff *pvidiff) +static int ixif_br_port_gen_pvid_del(struct lyd_node *cif, const char *brname, int vid) { - struct lyd_node *bridge = lydx_find_by_name(lyd_parent(cif), "interface", brname); - struct lyd_node *vlan, *vlans; - int err = 0; - FILE *br; + const char *iface, *mode; + FILE *exit; - vlans = lydx_get_descendant(lyd_child(bridge), "bridge", "vlans", NULL); - if (!vlans) - goto done; + iface = lydx_get_cattr(cif, "name"); - br = dagger_fopen_next(net, "init", brname, 60, "init.bridge"); - if (!br) { - err = -EIO; - goto done; - } + mode = ixif_br_port_get_egress_mode(iface, vid, brname); + if (!mode) + /* Port is not a member of the VLAN anymore, so the + * PVID is already removed. + */ + return 0; - LYX_LIST_FOR_EACH(lyd_child(vlans), vlan, "vlan") { - int vid = atoi(lydx_get_cattr(vlan, "vid")); + exit = dagger_fopen_current(&confd.netdag, "exit", brname, 61, "delete-pvids.bridge"); + if (!exit) + return -EIO; - brport_pvid_adjust(br, vlan, vid, brport, pvidiff, 0); - brport_pvid_adjust(br, vlan, vid, brport, pvidiff, 1); - } + /* Since PVID is a flag on the VLAN rather than a separate + * setting, the delete option becomes add-but-omit-pvid. + */ + fprintf(exit, "vlan add vid %d dev %s %s %s\n", + vid, iface, mode, !strcmp(brname, iface) ? "self" : ""); - fclose(br); -done: - return err; + fclose(exit); + return 0; } -static void bridge_port_settings(FILE *next, const char *ifname, struct lyd_node *cif) +static int ixif_br_port_gen_pvid_add(struct lyd_node *cif, const char *brname, int vid) +{ + const char *iface, *mode; + FILE *init; + + iface = lydx_get_cattr(cif, "name"); + + mode = ixif_br_port_get_egress_mode(iface, vid, brname); + if (!mode) { + WARN("%s is not a member of VLAN %d: Ignoring PVID", iface, vid); + return 0; + } + + init = dagger_fopen_next(&confd.netdag, "init", brname, 61, "add-pvids.bridge"); + if (!init) + return -EIO; + + fprintf(init, "vlan add vid %d dev %s pvid %s %s\n", + vid, iface, mode, !strcmp(brname, iface) ? "self" : ""); + + fclose(init); + return 0; +} + +static int ixif_br_port_gen_pvid(struct lyd_node *dif, struct lyd_node *cif) +{ + struct lyd_node *bridge, *pvid; + struct lydx_diff pvdiff; + const char *brname; + int err; + + pvid = lydx_get_descendant(lyd_child(dif), "bridge-port", "pvid", NULL); + if (!pvid || !lydx_get_diff(pvid, &pvdiff)) + return 0; + + brname = lydx_get_cattr(lydx_get_child(cif, "bridge-port"), "bridge"); + if (!brname) + /* The interface is itself a bridge. */ + brname = lydx_get_cattr(cif, "name"); + + bridge = lydx_get_descendant(lyd_child(dif), "bridge-port", "bridge", NULL); + + /* We only need to remove our old PVID in the case when this + * port has _not_ switched bridge. Otherwise, all old VLAN + * config will be removed as a result of detaching from the + * old bridge. + */ + if (!bridge && pvdiff.old) { + err = ixif_br_port_gen_pvid_del(cif, brname, atoi(pvdiff.old)); + if (err) + return err; + } + + if (pvdiff.new) { + err = ixif_br_port_gen_pvid_add(cif, brname, atoi(pvdiff.new)); + if (err) + return err; + } + + return 0; +} + +static int ixif_br_port_gen_link(struct lyd_node *dif, struct lyd_node *cif) { struct lyd_node *bp, *flood, *mcast; - int ucflood = 1; /* default: flood unknown unicast */ + const char *brname, *iface; + int mrouter; + FILE *next; + int err; - bp = lydx_get_descendant(lyd_child(cif), "bridge-port", NULL); + if (!lydx_get_child(dif, "bridge-port")) + return 0; + + bp = lydx_get_child(cif, "bridge-port"); if (!bp) - return; + return 0; + + brname = lydx_get_cattr(bp, "bridge"); + if (!brname) + /* The interface is itself a bridge. */ + return 0; + + iface = lydx_get_cattr(cif, "name"); + + err = dagger_add_dep(&confd.netdag, brname, iface); + if (err) + return ERR_IFACE(cif, err, "Unable to add dep \"%s\" to %s", iface, brname); + + next = dagger_fopen_next(&confd.netdag, "init", brname, 55, "add-ports.ip"); + if (!next) + return -EIO; + + fprintf(next, "link set %s type bridge_slave", iface); - fprintf(next, "link set %s type bridge_slave", ifname); flood = lydx_get_child(bp, "flood"); - if (flood) { - ucflood = lydx_is_enabled(flood, "unicast"); + fprintf(next, " bcast_flood %s", ONOFF(lydx_is_enabled(flood, "broadcast"))); + fprintf(next, " flood %s", ONOFF(lydx_is_enabled(flood, "unicast"))); + fprintf(next, " mcast_flood %s", ONOFF(lydx_is_enabled(flood, "multicast"))); - fprintf(next, " bcast_flood %s", ONOFF(lydx_is_enabled(flood, "broadcast"))); - fprintf(next, " flood %s", ONOFF(ucflood)); - fprintf(next, " mcast_flood %s", ONOFF(lydx_is_enabled(flood, "multicast"))); - } - - if (ucflood) { - /* proxy arp must be disabled while flood on, see man page */ + if (lydx_is_enabled(flood, "unicast")) { + /* proxy arp must be disabled while flood on, see man + * page. + */ fprintf(next, " proxy_arp off"); fprintf(next, " proxy_arp_wifi off"); } else { @@ -91,104 +167,71 @@ static void bridge_port_settings(FILE *next, const char *ifname, struct lyd_node } mcast = lydx_get_child(bp, "multicast"); - if (mcast) { - const char *router = lydx_get_cattr(mcast, "router"); - struct { const char *str; int val; } xlate[] = { - { "off", 0 }, - { "auto", 1 }, - { "permanent", 2 }, - }; - int mrouter = 1; - for (size_t i = 0; i < NELEMS(xlate); i++) { - if (strcmp(xlate[i].str, router)) - continue; + mrouter = 1; + if (!strcmp(lydx_get_cattr(mcast, "router"), "off")) + mrouter = 0; + else if (!strcmp(lydx_get_cattr(mcast, "router"), "permanent")) + mrouter = 2; - mrouter = xlate[i].val; - break; - } + fprintf(next, " mcast_fast_leave %s mcast_router %d", + ONOFF(lydx_is_enabled(mcast, "fast-leave")), mrouter); - fprintf(next, " mcast_fast_leave %s mcast_router %d", - ONOFF(lydx_is_enabled(mcast, "fast-leave")), - mrouter); - } fprintf(next, "\n"); + fclose(next); + return 0; } -int bridge_gen_ports(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip) +int ixif_br_port_gen_join_leave(struct lyd_node *dif) { - const char *ifname = lydx_get_cattr(cif, "name"); - struct lyd_node *node, *bridge; + struct lyd_node *bridge; struct lydx_diff brdiff; + const char *iface; + FILE *prev, *next; int err = 0; - node = lydx_get_descendant(lyd_child(dif), "bridge-port", NULL); - if (!node) - goto fail; + bridge = lydx_get_descendant(lyd_child(dif), "bridge-port", "bridge", NULL); + if (!bridge || !lydx_get_diff(bridge, &brdiff)) + return 0; - /* - * If bridge is not in dif, then we only have bridge-port - * settings and can use cif instead for any new settings - * since we always set *all* port settings anyway. - */ - bridge = lydx_get_child(node, "bridge"); - if (!bridge) { - struct lyd_node *pvid = lydx_get_child(node, "pvid"); - struct lydx_diff pvidiff; - const char *brname; - FILE *next; + iface = lydx_get_cattr(dif, "name"); - node = lydx_get_descendant(lyd_child(cif), "bridge-port", NULL); - brname = lydx_get_cattr(node, "bridge"); - if (!node || !brname) - goto fail; + if (brdiff.old) { + prev = dagger_fopen_current(&confd.netdag, "exit", brdiff.old, 55, "delete-ports.ip"); + if (!prev) + return -EIO; - next = dagger_fopen_next(net, "init", ifname, 56, "init.ip"); - if (!next) { - err = -EIO; - goto fail; - } - bridge_port_settings(next, ifname, cif); - fclose(next); - - /* Change in bridge port's PVID => change in VLAN port memberships */ - if (lydx_get_diff(pvid, &pvidiff)) - bridge_port_vlans(net, cif, brname, ifname, &pvidiff); - - err = dagger_add_dep(net, brname, ifname); - if (err) - return ERR_IFACE(cif, err, "Unable to add dep \"%s\" to %s", ifname, brname); - goto fail; - } - - if (lydx_get_diff(bridge, &brdiff) && brdiff.old) { - FILE *prev; - - prev = dagger_fopen_current(net, "exit", brdiff.old, 55, "exit.ip"); - if (!prev) { - err = -EIO; - goto fail; - } - fprintf(prev, "link set %s nomaster\n", ifname); + fprintf(prev, "link set %s nomaster\n", iface); fclose(prev); } if (brdiff.new) { - FILE *next; + next = dagger_fopen_next(&confd.netdag, "init", brdiff.new, 55, "add-ports.ip"); + if (!next) + return -EIO; - next = dagger_fopen_next(net, "init", brdiff.new, 55, "init.ip"); - if (!next) { - err = -EIO; - goto fail; - } - fprintf(next, "link set %s master %s\n", ifname, brdiff.new); - bridge_port_settings(next, ifname, cif); + fprintf(next, "link set %s master %s\n", iface, brdiff.new); fclose(next); - - err = dagger_add_dep(net, brdiff.new, ifname); - if (err) - return ERR_IFACE(cif, err, "Unable to add dep \"%s\" to %s", ifname, brdiff.new); } -fail: + return err; } + +int ixif_br_port_gen(struct lyd_node *dif, struct lyd_node *cif, FILE *ip) +{ + int err = 0; + + err = ixif_br_port_gen_join_leave(dif); + if (err) + return err; + + err = ixif_br_port_gen_link(dif, cif); + if (err) + return err; + + err = ixif_br_port_gen_pvid(dif, cif); + if (err) + return err; + + return 0; +}