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

Rauta, Alin alin.rauta at intel.com
Mon Feb 16 05:53:58 PST 2015


Hi Tom,

Regarding your request:

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

This is a little bit tricky.
When we bring the link down it would be easy to change the state to  LINK_STATE_DOWN, but when we bring the port up I don’t know how to restore it to the previous state unless I use an additional variable in the link structure to retain the previous state of the port, but this will not look nice I think. 
Would this be acceptable ?

Currently, when a link becomes down, it will have the operational state off. See " State: off (configured)" below:
Would this suffice since however we don't have a corresponding LINK_STATE_UP and from current behavior it seems acceptable to have the port down (operational "off") and "configured".

# networkctl status sw0p3
● 5: sw0p3
       Link File: /usr/lib/systemd/network/99-default.link
    Network File: /etc/systemd/network/sw0p3.network
            Type: ether
           State: off (configured)
          Driver: fm6k
             MTU: 1500
Carrier Bound To: sw0p2
                  sw0p1
#

Best Regards,
Alin

-----Original Message-----
From: Zbigniew Jędrzejewski-Szmek [mailto:zbyszek at in.waw.pl] 
Sent: Saturday, February 14, 2015 3:34 PM
To: Rauta, Alin
Cc: Tom Gundersen; Lennart Poettering; systemd Mailing List; Kinsella, Ray
Subject: Re: [PATCH v3] Added support for Uplink Failure Detection using BindCarrier

On Sat, Feb 14, 2015 at 03:26:00PM +0000, Rauta, Alin wrote:
> Hi guys,
> 
> Thanks for your input on this. Much appreciated.
> I'll try handle your remarks in the next patch.
> Please find my comments below.
> 
> Best Regards,
> Alin
> 
> > -----Original Message-----
> > From: Tom Gundersen [mailto:teg at jklm.no]
> > Sent: Saturday, February 14, 2015 2:50 PM
> > To: Zbigniew Jędrzejewski-Szmek
> > Cc: Rauta, Alin; Lennart Poettering; systemd Mailing List; Kinsella, 
> > Ray
> > Subject: Re: [PATCH v3] Added support for Uplink Failure Detection 
> > using BindCarrier
> 
> > On Sat, Feb 14, 2015 at 3:05 PM, Zbigniew Jędrzejewski-Szmek <zbyszek at in.waw.pl> wrote:
> > On Fri, Feb 13, 2015 at 10:27:07PM +0100, Tom Gundersen wrote:
> >> 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
> > "operational down" does not follow normal grammar rules. "are in an 
> > operational down state"?
> >
> >> > +            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.
> >>
> 
> 
> Alin: 
> I will think of something else here. Some other way to describe this.
> 
> 
> 
> >> > +          </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).
> >>
> 
> 
> Alin:
> I will change the name and provide functions in both directions.
> 
> 
> >> >  _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;
> > Why is c size_t?
> 
> 
> That's size_t because there is a similar way of using it in the same file. See "decode_and_sort_links" function:
> "size_t size = 0, c = 0;"
Please change both. Because this is size_t, you need a cast later, which is something to be avoided.

> >> > +        int r;
> >> > +
> >> > +        assert(m);
> >> > +        assert(name);
> >> > +
> >> > +        *down_count = 0;
> >> > +        *downlinks = NULL;
> > Why do you initialize this here? If an error happens, it is nice to 
> > not modify output parameters at all.
> >
> 
> Alin: 
> I modified the output parameters here so that in case of errors to be returned as 0 and NULL, but I can change that.
Yeah, it wasn't wrong, but don't-modify-anything-on-error is the standard that Lennart likes, and it is good to have things uniform.

Zbyszek

> >> > +        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.
> >>
> 
> Alin:
> I'll try.
> 
> 
> 
> >> > +        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->network->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?
> 
> Alin:
> It makes sence. This will require some changes.
> For simplicity and because I kept only references to "BindCarrier=" lists, I evaluated the lists dinamically and in full each time an event happens. I thought that was suggested.
> 
> 
> >>
> >> >  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?
> >>
> 
> Alin:
> "hashmap_first" was used because in case of link drop events the current link pointer may be NULL due to "link_unref" call or at least that was my impression.
> 
> 
> >> >          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;
> > Maybe just log a warning and drop it from the list? Seems like this 
> > could be common enough error with globs, but probably not worth refusing to parse the configuration over.
> 
> > Oh, this won't really work I think. There is no reason that *carrier should match match_name (this may be unset, or a glob itself). If we want to detect self-references we should be doing this at runtime I think, once we know how the matches have been resolved.
> 
> Alin:
> I will change it then.
> 
> >> > +                        }
> >> > +
> >> >          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);
> >
> > Zbyszek
> 
> Best Regards,
> Alin
> 
> 


More information about the systemd-devel mailing list