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

[Rauta, Alin]

Hi Lennart,

> +_public_ int sd_network_link_get_carriers(int ifindex, char ***ret) {
> +        return network_get_link_strv("CARRIERS", ifindex, ret); }
> +

>  I think it would be better to have two calls here:
>
>   int sd_network_link_get_carrier_bound_to(int ifindex, int **others);
>   int sd_network_link_get_carrier_bound_by(int ifindex, int **others);

In terms of functionality, " sd_network_link_get_carriers " is actually " sd_network_link_get_carrier_bound_to" and is applicable to "bound to" links just like "BindCarrier=" is available only for "bound to" links. I wanted to save to systemd "run" files just minimum info.

If I add "sd_network_link_get_carrier_bound_by", then each time "link_save" is called for a link I should query "BindCarrier="s for all interfaces to print each link that bounds the current interface.
Then, if I rename a link I should call "link_save" for all available links because the "BindCarrier=" can be interpreted in another way due to wildcards.

I would prefer using only one function and change the name as you suggested "sd_network_link_get_carrier_bound_to". Do you agree ? 

> +/* 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) {

Regarding "get_uplinks" and "get_downlinks" I can rename them to "get_links_bound_to" and "get_links_bound_by". Would this be fine ?

Best Regards,
Alin

-----Original Message-----
From: Lennart Poettering [mailto:lennart at poettering.net] 
Sent: Wednesday, February 11, 2015 4:54 PM
To: Rauta, Alin
Cc: zbyszek at in.waw.pl; teg at jklm.no; systemd-devel at lists.freedesktop.org; Kinsella, Ray
Subject: Re: [PATCH v3] Added support for Uplink Failure Detection using BindCarrier

On Tue, 10.02.15 03:30, 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>
> +          </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 think it would be better to have two calls here:

    int sd_network_link_get_carrier_bound_to(int ifindex, int **others);
    int sd_network_link_get_carrier_bound_by(int ifindex, int **others);

This would then allow querying the list of interfaces currently bound to the carrier of the interface you specify, as well as the list of interfaces the network currently applied to the interface you specify is bound to.

> +/* 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) {


Hmm, maybe we should agree on one nomenclature here.

So far I kinda preferred calling these things "bound by" and "bound to". I figure the terms "downlinks" and "uplink" are common too. We should really stick to one set of terms here. I personally find "bound by" and "bound to" more descriptive I must say.

I'll leave the rest to Tom.

Lennart

--
Lennart Poettering, Red Hat