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.ra...@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.
>
> > + </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;
Why is c size_t?
> > + 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.
> > + 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;
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.
> > + }
> > +
> > 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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
