[systemd-devel] [PATCH v3] Added support for Uplink Failure Detection using BindCarrier

Tom Gundersen teg at jklm.no
Fri Feb 13 13:27:07 PST 2015


Hi Alin,

Thanks for the patch. This is starting to look pretty good now.

I still have some questions/requests regarding some implementation
details (below), but hopefully we can get this merged after the next
release (trying to stabilize things at the moment).

On Tue, Feb 10, 2015 at 12:30 PM, Alin Rauta <alin.rauta at intel.com> wrote:
> ---
>  man/systemd.network.xml                  |  11 ++
>  src/libsystemd/sd-network/sd-network.c   |   4 +
>  src/network/networkctl.c                 | 211 ++++++++++++++++++++++++++++---
>  src/network/networkd-link.c              | 123 +++++++++++++++++-
>  src/network/networkd-link.h              |   1 +
>  src/network/networkd-manager.c           |   7 +
>  src/network/networkd-network-gperf.gperf |   1 +
>  src/network/networkd-network.c           |  10 ++
>  src/network/networkd.h                   |   2 +-
>  src/systemd/sd-network.h                 |   3 +
>  10 files changed, 355 insertions(+), 18 deletions(-)
>
> diff --git a/man/systemd.network.xml b/man/systemd.network.xml
> index 9b3a92d..8b2ca4e 100644
> --- a/man/systemd.network.xml
> +++ b/man/systemd.network.xml
> @@ -270,6 +270,17 @@
>            </listitem>
>          </varlistentry>
>          <varlistentry>
> +          <term><varname>BindCarrier=</varname></term>
> +          <listitem>
> +            <para>A port or a list of ports. When set, controls the
> +            behaviour of the current interface. When all ports in the list
> +            are operational down, the failure is propagated to the current
> +            interface. When at least one port has carrier, the current
> +            interface is brought up.
> +            </para>

Maybe we should make it clear that this is not necessarily a failure
(the uplink may be down on purpose), and that the way we propagate it
is to set the downlink administrative down.

> +          </listitem>
> +        </varlistentry>
> +        <varlistentry>
>            <term><varname>Address=</varname></term>
>            <listitem>
>              <para>A static IPv4 or IPv6 address and its prefix length,
> diff --git a/src/libsystemd/sd-network/sd-network.c b/src/libsystemd/sd-network/sd-network.c
> index c735cac..b574d19 100644
> --- a/src/libsystemd/sd-network/sd-network.c
> +++ b/src/libsystemd/sd-network/sd-network.c
> @@ -264,6 +264,10 @@ _public_ int sd_network_link_get_domains(int ifindex, char ***ret) {
>          return network_get_link_strv("DOMAINS", ifindex, ret);
>  }
>
> +_public_ int sd_network_link_get_carriers(int ifindex, char ***ret) {
> +        return network_get_link_strv("CARRIERS", ifindex, ret);
> +}

I don't find get_carriers() very clear. At least call it
get_carrier_bound_to() or something like that. I must say I agree with
Lennart and Zbigniew, we should introduce the API in both directions,
and then we can worry about the performance if that turns out to be a
problem (worst case all the processing could be hidden in the
sd-network library, but I don't think that will be necessary to be
honest).

>  _public_ int sd_network_link_get_wildcard_domain(int ifindex) {
>          int r;
>          _cleanup_free_ char *p = NULL, *s = NULL;
> diff --git a/src/network/networkctl.c b/src/network/networkctl.c
> index aa83f32..ffb38e8 100644
> --- a/src/network/networkctl.c
> +++ b/src/network/networkctl.c
> @@ -22,6 +22,7 @@
>  #include <stdbool.h>
>  #include <getopt.h>
>  #include <net/if.h>
> +#include <fnmatch.h>
>
>  #include "sd-network.h"
>  #include "sd-rtnl.h"
> @@ -393,6 +394,161 @@ static int get_gateway_description(
>          return -ENODATA;
>  }
>
> +static bool is_carrier(const char *ifname,
> +                       char **carriers) {
> +       char **i;
> +
> +       STRV_FOREACH(i, carriers)
> +               if (fnmatch(*i, ifname, 0) == 0)
> +                       return true;
> +
> +       return false;
> +}
> +
> +/* get the links that are bound to this port. */
> +static int get_downlinks(const char *name,
> +                         sd_rtnl_message *m,
> +                         LinkInfo **downlinks,
> +                         int *down_count) {
> +        _cleanup_free_ LinkInfo  *links = NULL;
> +        sd_rtnl_message *i;
> +        size_t size = 0;
> +        size_t c = 0;
> +        int r;
> +
> +        assert(m);
> +        assert(name);
> +
> +        *down_count = 0;
> +        *downlinks = NULL;
> +
> +        for (i = m; i; i = sd_rtnl_message_next(i)) {
> +                _cleanup_strv_free_ char **carriers = NULL;
> +                const char *link_name;
> +                int ifindex;
> +
> +                r = sd_rtnl_message_link_get_ifindex(i, &ifindex);
> +                if (r < 0)
> +                        return r;
> +
> +                r = sd_rtnl_message_read_string(i, IFLA_IFNAME, &link_name);
> +                if (r < 0)
> +                        return r;
> +
> +                r = sd_network_link_get_carriers(ifindex, &carriers);
> +                if (r == -ENODATA || strv_isempty(carriers))
> +                        continue;
> +
> +                if (r < 0) {
> +                        log_warning("Failed to get carrier list for port: %s", name);
> +                        continue;
> +                }
> +
> +                if (is_carrier(name, carriers)) {
> +                         if (!GREEDY_REALLOC(links, size, c+1))
> +                                 return -ENOMEM;
> +
> +                         links[c].name = link_name;
> +                         c++;
> +                }
> +        }
> +
> +        *downlinks = links;
> +        *down_count = (int) c;
> +
> +        links = NULL;
> +
> +        return 0;
> +}
> +
> +/* get the links to which current port is bound to. */
> +static int get_uplinks(int ifindex,
> +                       sd_rtnl_message *m,
> +                       LinkInfo **uplinks,
> +                       int *up_count) {
> +        _cleanup_free_ LinkInfo  *links = NULL;
> +        _cleanup_strv_free_ char **carriers = NULL;
> +        sd_rtnl_message *i;
> +        size_t size = 0, c = 0;
> +        int r;
> +
> +        assert(m);
> +
> +        *up_count = 0;
> +        *uplinks = NULL;
> +
> +        /* check if this port has carriers. */
> +        r = sd_network_link_get_carriers(ifindex, &carriers);
> +        if (r == -ENODATA || strv_isempty(carriers))
> +                return 0;
> +
> +        if (r < 0)
> +                return r;
> +
> +        for (i = m; i; i = sd_rtnl_message_next(i)) {
> +                const char *name;
> +
> +                r = sd_rtnl_message_read_string(i, IFLA_IFNAME, &name);
> +                if (r < 0)
> +                        return r;
> +
> +                if (is_carrier(name, carriers)) {
> +                         if (!GREEDY_REALLOC(links, size, c+1))
> +                                 return -ENOMEM;
> +
> +                         links[c].name = name;
> +                         c++;
> +                }
> +        }
> +
> +        *uplinks = links;
> +        *up_count = (int) c;
> +
> +        links = NULL;
> +
> +        return 0;
> +}
> +
> +static int get_links(sd_rtnl *rtnl,
> +                     sd_rtnl_message **ret) {
> +        _cleanup_rtnl_message_unref_ sd_rtnl_message *req = NULL, *reply = NULL;
> +        int r;
> +        assert(rtnl);
> +
> +        r = sd_rtnl_message_new_link(rtnl, &req, RTM_GETLINK, 0);
> +        if (r < 0)
> +                return rtnl_log_create_error(r);
> +
> +        r = sd_rtnl_message_request_dump(req, true);
> +        if (r < 0)
> +                return rtnl_log_create_error(r);
> +
> +        r = sd_rtnl_call(rtnl, req, 0, &reply);
> +        if (r < 0) {
> +                log_error("Failed to enumerate links: %s", strerror(-r));
> +                return r;
> +        }
> +
> +        *ret = reply;
> +        reply = NULL;
> +
> +        return 0;
> +}
> +
> +static void dump_carrier_list(const char *prefix,
> +                              LinkInfo *links,
> +                              int len) {
> +        int i;
> +
> +        assert(links);
> +
> +        for (i = 0; i < len; i++)
> +                printf("%*s%s\n",
> +                       (int) strlen(prefix),
> +                       i == 0 ? prefix : "",
> +                       links[i].name);
> +}
> +
>  static int dump_gateways(
>                  sd_rtnl *rtnl,
>                  sd_hwdb *hwdb,
> @@ -508,11 +664,16 @@ static int link_status_one(
>          const char *driver = NULL, *path = NULL, *vendor = NULL, *model = NULL, *link = NULL;
>          const char *on_color_operational, *off_color_operational,
>                     *on_color_setup, *off_color_setup;
> +        _cleanup_rtnl_message_unref_ sd_rtnl_message *link_list = NULL;
> +        _cleanup_free_ LinkInfo *uplinks = NULL;    /* links to which current port is bound to. */
> +        _cleanup_free_ LinkInfo *downlinks = NULL;  /* links that are bound to current port. */
>          struct ether_addr e;
>          unsigned iftype;
>          int r, ifindex;
>          bool have_mac;
>          uint32_t mtu;
> +        int up_count;
> +        int down_count;
>
>          assert(rtnl);
>          assert(udev);
> @@ -606,12 +767,24 @@ static int link_status_one(
>
>          sd_network_link_get_network_file(ifindex, &network);
>
> +        r = get_links(rtnl, &link_list);
> +        if (r < 0)
> +                return r;
> +
> +        r = get_uplinks(ifindex, link_list, &uplinks, &up_count);
> +        if (r < 0)
> +                log_warning("Failed to get uplinks for port: %d", ifindex);
> +
> +        r = get_downlinks(name, link_list, &downlinks, &down_count);
> +        if (r < 0)
> +                log_warning("Failed to get downlinks for port: %d", ifindex);
> +
>          printf("%s%s%s %i: %s\n", on_color_operational, draw_special_char(DRAW_BLACK_CIRCLE), off_color_operational, ifindex, name);
>
> -        printf("   Link File: %s\n"
> -               "Network File: %s\n"
> -               "        Type: %s\n"
> -               "       State: %s%s%s (%s%s%s)\n",
> +        printf("       Link File: %s\n"
> +               "    Network File: %s\n"
> +               "            Type: %s\n"
> +               "           State: %s%s%s (%s%s%s)\n",
>                 strna(link),
>                 strna(network),
>                 strna(t),
> @@ -619,13 +792,13 @@ static int link_status_one(
>                 on_color_setup, strna(setup_state), off_color_setup);
>
>          if (path)
> -                printf("        Path: %s\n", path);
> +                printf("            Path: %s\n", path);
>          if (driver)
> -                printf("      Driver: %s\n", driver);
> +                printf("          Driver: %s\n", driver);
>          if (vendor)
> -                printf("      Vendor: %s\n", vendor);
> +                printf("          Vendor: %s\n", vendor);
>          if (model)
> -                printf("       Model: %s\n", model);
> +                printf("           Model: %s\n", model);
>
>          if (have_mac) {
>                  _cleanup_free_ char *description = NULL;
> @@ -634,23 +807,29 @@ static int link_status_one(
>                  ieee_oui(hwdb, &e, &description);
>
>                  if (description)
> -                        printf("  HW Address: %s (%s)\n", ether_addr_to_string(&e, ea), description);
> +                        printf("      HW Address: %s (%s)\n", ether_addr_to_string(&e, ea), description);
>                  else
> -                        printf("  HW Address: %s\n", ether_addr_to_string(&e, ea));
> +                        printf("      HW Address: %s\n", ether_addr_to_string(&e, ea));
>          }
>
>          if (mtu > 0)
> -                printf("         MTU: %u\n", mtu);
> +                printf("             MTU: %u\n", mtu);
>
> -        dump_addresses(rtnl, "     Address: ", ifindex);
> -        dump_gateways(rtnl, hwdb, "     Gateway: ", ifindex);
> +        dump_addresses(rtnl, "         Address: ", ifindex);
> +        dump_gateways(rtnl, hwdb, "         Gateway: ", ifindex);
>
>          if (!strv_isempty(dns))
> -                dump_list("         DNS: ", dns);
> +                dump_list("             DNS: ", dns);
>          if (!strv_isempty(domains))
> -                dump_list("      Domain: ", domains);
> +                dump_list("          Domain: ", domains);
>          if (!strv_isempty(ntp))
> -                dump_list("         NTP: ", ntp);
> +                dump_list("             NTP: ", ntp);
> +
> +        if (uplinks)
> +                dump_carrier_list("Carrier Bound To: ", uplinks, up_count);
> +
> +        if (downlinks)
> +                dump_carrier_list("Carrier Bound By: ", downlinks, down_count);
>
>          return 0;
>  }
> diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
> index 0b1cac1..ccd255a 100644
> --- a/src/network/networkd-link.c
> +++ b/src/network/networkd-link.c
> @@ -22,6 +22,7 @@
>  #include <netinet/ether.h>
>  #include <linux/if.h>
>  #include <unistd.h>
> +#include <fnmatch.h>
>
>  #include "util.h"
>  #include "virt.h"
> @@ -1151,6 +1152,58 @@ static int link_up(Link *link) {
>          return 0;
>  }
>
> +static int link_down_handler(sd_rtnl *rtnl, sd_rtnl_message *m, void *userdata) {
> +        _cleanup_link_unref_ Link *link = userdata;
> +        int r;
> +
> +        assert(link);
> +
> +        if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER))
> +                return 1;
> +
> +        r = sd_rtnl_message_get_errno(m);
> +        if (r < 0)
> +                log_link_warning_errno(link, -r, "%-*s: could not bring down interface: %m", IFNAMSIZ, link->ifname);
> +
> +        return 1;
> +}
> +
> +static int link_down(Link *link) {
> +        _cleanup_rtnl_message_unref_ sd_rtnl_message *req = NULL;
> +        int r;
> +
> +        assert(link);
> +        assert(link->manager);
> +        assert(link->manager->rtnl);
> +
> +        r = sd_rtnl_message_new_link(link->manager->rtnl, &req,
> +                                     RTM_SETLINK, link->ifindex);
> +        if (r < 0) {
> +                log_link_error(link, "Could not allocate RTM_SETLINK message");
> +                return r;
> +        }
> +
> +        r = sd_rtnl_message_link_set_flags(req, 0, IFF_UP);
> +        if (r < 0) {
> +                log_link_error(link, "Could not set link flags: %s",
> +                               strerror(-r));
> +                return r;
> +        }
> +
> +        r = sd_rtnl_call_async(link->manager->rtnl, req, link_down_handler, link,
> +                               0, NULL);
> +        if (r < 0) {
> +                log_link_error(link,
> +                               "Could not send rtnetlink message: %s",
> +                               strerror(-r));
> +                return r;
> +        }

Hm, we need to introduce a new administrative state here I think
(LINK_STATE_DOWN), and then make sure we don't accidentally leave it
in case we get some other event after bringing the interface down.

> +        link_ref(link);
> +
> +        return 0;
> +}
> +
>  static int link_joined(Link *link) {
>          int r;
>
> @@ -1982,7 +2035,7 @@ int link_save(Link *link) {
>                  admin_state, oper_state);
>
>          if (link->network) {
> -                char **address, **domain;
> +                char **address, **domain, **carrier;
>                  bool space;
>
>                  fprintf(f, "NETWORK_FILE=%s\n", link->network->filename);
> @@ -2061,6 +2114,15 @@ int link_save(Link *link) {
>
>                  fprintf(f, "LLMNR=%s\n",
>                          llmnr_support_to_string(link->network->llmnr));
> +
> +                fputs("CARRIERS=", f);
> +                space = false;
> +                STRV_FOREACH(carrier, link->network->carriers) {
> +                        if (space)
> +                                fputc(' ', f);
> +                        fputs(*carrier, f);
> +                        space = true;
> +                }
>          }
>
>          if (link->dhcp_lease) {
> @@ -2106,6 +2168,65 @@ fail:
>          return r;
>  }
>
> +int link_handle_carriers(Link *link) {
> +        Manager *m;
> +        char **carrier;
> +        Link *downlink;
> +        Link *uplink;
> +        Iterator i;
> +        Iterator j;
> +        int r;
> +
> +        assert(link);
> +        assert(link->manager);
> +
> +        m = link->manager;
> +
> +        /* go through the list of available links. */
> +        HASHMAP_FOREACH (downlink, m->links, i) {
> +                if (downlink->network && !strv_isempty(downlink->network->carriers)) {
> +                        bool has_carrier;
> +                        bool required_up = false;
> +                        bool found_match = false;
> +
> +                        has_carrier = link_has_carrier(downlink);
> +
> +                        STRV_FOREACH (carrier, downlink->network->carriers) {
> +                                HASHMAP_FOREACH (uplink, m->links, j) {
> +                                        if (fnmatch(*carrier, uplink->ifname, 0) == 0) {
> +                                                found_match = true;
> +
> +                                                if (link_has_carrier(uplink)) {
> +                                                        required_up = true;
> +                                                        break;
> +                                                }
> +                                        }
> +                                }
> +
> +                                if (required_up)
> +                                        break;
> +                        }
> +
> +                        if (found_match) {
> +                                if (has_carrier && !required_up) {
> +                                        r = link_down(downlink);
> +                                        if (r < 0)
> +                                                return r;
> +                                }
> +                                else if (!has_carrier && required_up) {
> +                                        if (!(downlink->flags & IFF_UP)) {
> +                                                r = link_up(downlink);
> +                                                if (r < 0)
> +                                                        return r;
> +                                        }
> +                                }
> +                        }
> +                }
> +        }
> +
> +        return 0;
> +}

I would split this in two.

First, whenever a link is added/removed resolve the globbing and store
in each Link object pointers to its associated down/uplinks.

Second, whenever an uplink gains/loses its carrier, triggers each of
its associated downlinks (which have already been resolved) to go
through each of their uplinks (also already resolved) to see if any of
them has a carrier, and act accordingly.

Makes sense?

>  static const char* const link_state_table[_LINK_STATE_MAX] = {
>          [LINK_STATE_PENDING] = "pending",
>          [LINK_STATE_ENSLAVING] = "configuring",
> diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h
> index 449dbc8..d94e9cd 100644
> --- a/src/network/networkd-link.h
> +++ b/src/network/networkd-link.h
> @@ -106,6 +106,7 @@ int link_save(Link *link);
>
>  int link_carrier_reset(Link *link);
>  bool link_has_carrier(Link *link);
> +int link_handle_carriers(Link *link);
>
>  int link_set_mtu(Link *link, uint32_t mtu);
>  int link_set_hostname(Link *link, const char *hostname);
> diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c
> index 4617f11..06807df 100644
> --- a/src/network/networkd-manager.c
> +++ b/src/network/networkd-manager.c
> @@ -356,6 +356,13 @@ static int manager_rtnl_process_link(sd_rtnl *rtnl, sd_rtnl_message *message, vo
>                  assert_not_reached("Received invalid RTNL message type.");
>          }
>
> +        link = hashmap_first(m->links);
> +        if (link) {
> +                r = link_handle_carriers(link);
> +                if (r < 0)
> +                        return 0;
> +        }

Hm, this looks really wrong. Why hashmap_first()? What if this was a
change or remove event, hashmp_first() will have nothing to do with
the link in question, no? How about splitting this up as suggested
above?

>          return 1;
>  }
>
> diff --git a/src/network/networkd-network-gperf.gperf b/src/network/networkd-network-gperf.gperf
> index 1731e04..e191380 100644
> --- a/src/network/networkd-network-gperf.gperf
> +++ b/src/network/networkd-network-gperf.gperf
> @@ -48,6 +48,7 @@ Network.LLMNR,               config_parse_llmnr,                 0,
>  Network.NTP,                 config_parse_strv,                  0,                             offsetof(Network, ntp)
>  Network.IPForward,           config_parse_address_family_boolean,0,                             offsetof(Network, ip_forward)
>  Network.IPMasquerade,        config_parse_bool,                  0,                             offsetof(Network, ip_masquerade)
> +Network.BindCarrier,         config_parse_strv,                  0,                             offsetof(Network, carriers)
>  Address.Address,             config_parse_address,               0,                             0
>  Address.Peer,                config_parse_address,               0,                             0
>  Address.Broadcast,           config_parse_broadcast,             0,                             0
> diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c
> index 3ebd4d7..9c4425e 100644
> --- a/src/network/networkd-network.c
> +++ b/src/network/networkd-network.c
> @@ -21,6 +21,7 @@
>
>  #include <ctype.h>
>  #include <net/if.h>
> +#include <fnmatch.h>
>
>  #include "path-util.h"
>  #include "conf-files.h"
> @@ -37,6 +38,7 @@ static int network_load_one(Manager *manager, const char *filename) {
>          char *d;
>          Route *route;
>          Address *address;
> +        char **carrier;
>          int r;
>
>          assert(manager);
> @@ -154,6 +156,13 @@ static int network_load_one(Manager *manager, const char *filename) {
>                  }
>          }
>
> +        if (!strv_isempty(network->carriers))
> +                STRV_FOREACH(carrier, network->carriers)
> +                        if (fnmatch(*carrier, network->match_name, 0) == 0) {
> +                                log_error("Link bound to itself: %s", network->match_name);
> +                                return -EINVAL;
> +                        }
> +
>          network = NULL;
>
>          return 0;
> @@ -209,6 +218,7 @@ void network_free(Network *network) {
>          strv_free(network->ntp);
>          strv_free(network->dns);
>          strv_free(network->domains);
> +        strv_free(network->carriers);
>
>          netdev_unref(network->bridge);
>
> diff --git a/src/network/networkd.h b/src/network/networkd.h
> index 22cc51d..04596a8 100644
> --- a/src/network/networkd.h
> +++ b/src/network/networkd.h
> @@ -151,7 +151,7 @@ struct Network {
>          Hashmap *fdb_entries_by_section;
>
>          bool wildcard_domain;
> -        char **domains, **dns, **ntp;
> +        char **domains, **dns, **ntp, **carriers;
>
>          LLMNRSupport llmnr;
>
> diff --git a/src/systemd/sd-network.h b/src/systemd/sd-network.h
> index 027730d..0a7adf2 100644
> --- a/src/systemd/sd-network.h
> +++ b/src/systemd/sd-network.h
> @@ -116,6 +116,9 @@ int sd_network_link_get_lldp(int ifindex, char **lldp);
>  /* Get the DNS domain names for a given link. */
>  int sd_network_link_get_domains(int ifindex, char ***domains);
>
> +/* Get the CARRIERS to which current link is bound to. */
> +int sd_network_link_get_carriers(int ifindex, char ***carriers);
> +
>  /* Returns whether or not domains that don't match any link should be resolved
>   * on this link. 1 for yes, 0 for no and negative value for error */
>  int sd_network_link_get_wildcard_domain(int ifindex);
> --
> 1.9.3
>


More information about the systemd-devel mailing list