On Sat, Feb 14, 2015 at 3:05 PM, Zbigniew Jędrzejewski-Szmek <zbys...@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.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.
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. >> > + } >> > + >> > 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